8000 [PM-10388] Copy TOTP when autofilling by fedemkr · Pull Request #809 · bitwarden/ios · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[PM-10388] Copy TOTP when autofilling #809

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 9 commits into from
Aug 13, 2024
Merged

[PM-10388] Copy TOTP when autofilling #809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d769e06
PM-10388 Copies TOTP code if possible when autofilling. Moved the log…
fedemkr Aug 9, 2024
253ed7b
Merge branch 'main' into mobiletf/pm-10388/copy-totp-when-autofill-pa…
fedemkr Aug 9, 2024
2e9a87c
Update BitwardenShared/Core/Vault/Services/TOTP/TOTPServiceTests.swift
fedemkr Aug 12, 2024
471d074
Update BitwardenShared/Core/Vault/Services/TOTP/TOTPServiceTests.swift
fedemkr Aug 12, 2024
3efba45
Update BitwardenShared/Core/Vault/Services/TOTP/TOTPServiceTests.swift
fedemkr Aug 12, 2024
444fe48
Merge branch 'main' into mobiletf/pm-10388/copy-totp-when-autofill-pa…
fedemkr Aug 12, 2024
a1fa6d9
PM-10388 Tear down stateService on TOTPServiceTests and updated descr…
fedemkr Aug 12, 2024
ce8f55b
Merge branch 'main' into mobiletf/pm-10388/copy-totp-when-autofill-pa…
fedemkr Aug 12, 2024
20cd0ec
Merge branch 'main' into mobiletf/pm-10388/copy-totp-when-autofill-pa…
fedemkr Aug 12, 2024
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
23 changes: 16 additions & 7 deletions BitwardenShared/Core/Autofill/Services/AutofillCredentialService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ class DefaultAutofillCredentialService {
/// The service used to manage copy/pasting from the device's clipboard.
private let pasteboardService: PasteboardService

/// The service used by the application to validate TOTP keys and produce TOTP values
private let totpService: TOTPService

/// The service used by the application to manage account state.
private let stateService: StateService

Expand All @@ -113,6 +116,7 @@ class DefaultAutofillCredentialService {
/// - identityStore: The service used to manage the credentials available for AutoFill suggestions.
/// - pasteboardService: The service used to manage copy/pasting from the device's clipboard.
/// - stateService: The service used by the application to manage account state.
/// - totpService: The service used by the application to validate TOTP keys and produce TOTP values.
/// - vaultTimeoutService: The service used to manage vault access.
///
init(
Expand All @@ -125,6 +129,7 @@ class DefaultAutofillCredentialService {
identityStore: CredentialIdentityStore = ASCredentialIdentityStore.shared,
pasteboardService: PasteboardService,
stateService: StateService,
totpService: TOTPService,
vaultTimeoutService: VaultTimeoutService
) {
self.cipherService = cipherService
Expand All @@ -136,6 +141,7 @@ class DefaultAutofillCredentialService {
self.identityStore = identityStore
self.pasteboardService = pasteboardService
self.stateService = stateService
self.totpService = totpService
self.vaultTimeoutService = vaultTimeoutService

Task {
Expand Down Expand Up @@ -284,13 +290,10 @@ extension DefaultAutofillCredentialService: AutofillCredentialService {
throw ASExtensionError(.userInteractionRequired)
}

let disableAutoTotpCopy = try await stateService.getDisableAutoTotpCopy()
let accountHasPremium = try await stateService.doesActiveAccountHavePremium()
if !disableAutoTotpCopy,
let totp = cipher.login?.totp,
cipher.organizationUseTotp || accountHasPremium {
let codeModel = try await clientService.vault().generateTOTPCode(for: totp, date: nil)
pasteboardService.copy(codeModel.code)
do {
try await totpService.copyTotpIfPossible(cipher: cipher)
} catch {
errorReporter.log(error: error)
}

await eventService.collect(
Expand Down Expand Up @@ -380,6 +383,12 @@ extension DefaultAutofillCredentialService: AutofillCredentialService {
Fido2DebuggingReportBuilder.builder.withGetAssertionResult(.success(assertionResult))
#endif

do {
try await totpService.copyTotpIfPossible(cipher: assertionResult.selectedCredential.cipher)
} catch {
errorReporter.log(error: error)
}

return ASPasskeyAssertionCredential(
userHandle: assertionResult.userHandle,
relyingParty: rpId,
Expand Down
128 changes: 70 additions & 58 deletions BitwardenShared/Core/Autofill/Services/AutofillCredentialServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
var identityStore: MockCredentialIdentityStore!
var pasteboardService: MockPasteboardService!
var stateService: MockStateService!
var totpService: MockTOTPService!
var subject: DefaultAutofillCredentialService!
var vaultTimeoutService: MockVaultTimeoutService!

Expand All @@ -37,6 +38,7 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
identityStore = MockCredentialIdentityStore()
pasteboardService = MockPasteboardService()
stateService = MockStateService()
totpService = MockTOTPService()
vaultTimeoutService = MockVaultTimeoutService()

subject = DefaultAutofillCredentialService(
Expand All @@ -49,6 +51,7 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
identityStore: identityStore,
pasteboardService: pasteboardService,
stateService: stateService,
totpService: totpService,
vaultTimeoutService: vaultTimeoutService
)
}
Expand All @@ -67,6 +70,7 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
identityStore = nil
pasteboardService = nil
stateService = nil
totpService = nil
subject = nil
vaultTimeoutService = nil
}
Expand Down Expand Up @@ -208,12 +212,13 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t

XCTAssertEqual(credential.password, "password123")
XCTAssertEqual(credential.user, "user@bitwarden.com")
XCTAssertEqual(pasteboardService.copiedString, "123456")
XCTAssertTrue(totpService.copyTotpIfPossibleCalled)
}

/// `provideCredential(for:)` doesn't copy the cipher's TOTP code if the copy TOTP code setting
/// has been disabled.
func test_provideCredential_totpCopyDisabled() async throws {
/// `provideCredential(for:)` attempting to copy the cipher's TOTP code when returning the credential
/// throws when gettning if active account has premium thus it gets logged by the reporter
/// but the credential is still returned.
func test_provideCredential_totpCopyThrows() async throws {
cipherService.fetchCipherResult = .success(
.fixture(login: .fixture(
password: "password123",
Expand All @@ -222,8 +227,8 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
))
)
stateService.activeAccount = .fixture()
stateService.disableAutoTotpCopyByUserId["1"] = true
vaultTimeoutService.isClientLocked["1"] = false
totpService.copyTotpIfPossibleError = BitwardenTestError.example

let credential = try await subject.provideCredential(
for: "1",
Expand All @@ -233,59 +238,8 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t

XCTAssertEqual(credential.password, "password123")
XCTAssertEqual(credential.user, "user@bitwarden.com")
XCTAssertNil(pasteboardService.copiedString)
}

/// `provideCredential(for:)` doesn't copy the cipher's TOTP code if the user doesn't have premium access.
func test_provideCredential_totpCopyNotPremium() async throws {
cipherService.fetchCipherResult = .success(
.fixture(login: .fixture(
password: "password123",
username: "user@bitwarden.com",
totp: "totp"
))
)
stateService.activeAccount = .fixture()
stateService.doesActiveAccountHavePremiumResult = .success(false)
vaultTimeoutService.isClientLocked["1"] = false

let credential = try await subject.provideCredential(
for: "1",
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
repromptPasswordValidated: false
)

XCTAssertEqual(credential.password, "password123")
XCTAssertEqual(credential.user, "user@bitwarden.com")
XCTAssertNil(pasteboardService.copiedString)
}

/// `provideCredential(for:)` copies the cipher's TOTP code if the user doesn't have premium
/// but the org uses TOTP.
func test_provideCredential_totpCopyOrgUseTotp() async throws {
cipherService.fetchCipherResult = .success(
.fixture(
login: .fixture(
password: "password123",
username: "user@bitwarden.com",
totp: "totp"
),
organizationUseTotp: true
)
)
stateService.activeAccount = .fixture()
stateService.doesActiveAccountHavePremiumResult = .success(false)
vaultTimeoutService.isClientLocked["1"] = false

let credential = try await subject.provideCredential(
for: "1",
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
repromptPasswordValidated: false
)

XCTAssertEqual(credential.password, "password123")
XCTAssertEqual(credential.user, "user@bitwarden.com")
XCTAssertEqual(pasteboardService.copiedString, "123456")
XCTAssertTrue(totpService.copyTotpIfPossibleCalled)
XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example])
}

/// `provideCredential(for:)` throws an error if the user's vault is locked.
Expand Down Expand Up @@ -336,6 +290,9 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
XCTAssertFalse(autofillCredentialServiceDelegate.unlockVaultWithNeverlockKeyCalled)
XCTAssertEqual(fido2UserInterfaceHelper.userVerificationPreferenceSetup, .discouraged)

XCTAssertTrue(totpService.copyTotpIfPossibleCalled)
XCTAssertTrue(errorReporter.errors.isEmpty)

XCTAssertEqual(result.userHandle, expectedAssertionResult.userHandle)
XCTAssertEqual(result.relyingParty, passkeyIdentity.relyingPartyIdentifier)
XCTAssertEqual(result.signature, expectedAssertionResult.signature)
Expand All @@ -344,6 +301,61 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
XCTAssertEqual(result.credentialID, expectedAssertionResult.credentialId)
}

/// `provideFido2Credential(for:autofillCredentialServiceDelegate:fido2UserVerificationMediatorDelegate:)`
/// attempting to copy the cipher's TOTP code when returning the credential
/// throws when gettning if active account has premium thus it gets logged by the reporter
/// but the credential is still returned.
@available(iOS 17.0, *)
func test_provideFido2Credential_totpCopyThrows() async throws {
stateService.activeAccount = .fixture()
vaultTimeoutService.isClientLocked["1"] = false
let passkeyIdentity = ASPasskeyCredentialIdentity.fixture()
let passkeyRequest = ASPasskeyCredentialRequest.fixture(credentialIdentity: passkeyIdentity)
let expectedAssertionResult = GetAssertionResult.fixture(
selectedCredential: .fixture(
cipherView: .fixture(
login: .fixture(
totp: "totp"
)
)
)
)
totpService.copyTotpIfPossibleError = BitwardenTestError.example

clientService.mockPlatform.fido2Mock
.clientFido2AuthenticatorMock
.getAssertionMocker
.withVerification { request in
request.rpId == passkeyIdentity.relyingPartyIdentifier
&& request.clientDataHash == passkeyRequest.clientDataHash
&& request.allowList?[0].id == passkeyIdentity.credentialID
&& request.allowList?[0].ty == "public-key"
&& request.allowList?[0].transports == nil
&& !request.options.rk
&& request.options.uv == .discouraged
&& request.extensions == nil
}
.withResult(expectedAssertionResult)

let result = try await subject.provideFido2Credential(
for: passkeyRequest,
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
fido2UserInterfaceHelperDelegate: fido2UserInterfaceHelperDelegate
)

XCTAssertFalse(autofillCredentialServiceDelegate.unlockVaultWithNeverlockKeyCalled)
XCTAssertEqual(fido2UserInterfaceHelper.userVerificationPreferenceSetup, .discouraged)

XCTAssertEqual(result.userHandle, expectedAssertionResult.userHandle)
XCTAssertEqual(result.relyingParty, passkeyIdentity.relyingPartyIdentifier)
XCTAssertEqual(result.signature, expectedAssertionResult.signature)
XCTAssertEqual(result.clientDataHash, passkeyRequest.clientDataHash)
XCTAssertEqual(result.authenticatorData, expectedAssertionResult.authenticatorData)
XCTAssertEqual(result.credentialID, expectedAssertionResult.credentialId)
XCTAssertTrue(totpService.copyTotpIfPossibleCalled)
XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example])
}

/// `provideFido2Credential(for:autofillCredentialServiceDelegate:fido2UserVerificationMediatorDelegate:)`
/// succeeds when unlocking with never key.
@available(iOS 17.0, *)
Expand Down
9 changes: 7 additions & 2 deletions BitwardenShared/Core/Platform/Services/ServiceContainer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,6 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
timeProvider: timeProvider
)

let totpService = DefaultTOTPService()

let trustDeviceService = DefaultTrustDeviceService(
appIdService: appIdService,
authAPIService: apiService,
Expand All @@ -444,6 +442,12 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
stateService: stateService
)

let totpService = DefaultTOTPService(
clientService: clientService,
pasteboardService: pasteboardService,
stateService: stateService
)

let authService = DefaultAuthService(
accountAPIService: apiService,
appIdService: appIdService,
Expand Down Expand Up @@ -571,6 +575,7 @@ public class ServiceContainer: Services { // swiftlint:disable:this type_body_le
fido2UserInterfaceHelper: fido2UserInterfaceHelper,
pasteboardService: pasteboardService,
stateService: stateService,
totpService: totpService,
vaultTimeoutService: vaultTimeoutService
)

Expand Down
52 changes: 52 additions & 0 deletions BitwardenShared/Core/Vault/Services/TOTP/TOTPService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import BitwardenSdk
import Foundation

/// Protocol defining the functionality of a TOTP (Time-based One-Time Password) service.
protocol TOTPService {
/// Attempts to copy the totp to the clipboard if there is one to copy and the action is enabled.
/// - Parameter cipher: `CipherView` that contains totp information and permissions.
func copyTotpIfPossible(cipher: CipherView) async throws

/// Retrieves the TOTP configuration for a given key.
///
/// - Parameter key: A string representing the TOTP key.
Expand All @@ -12,6 +17,53 @@ protocol TOTPService {

/// Default implementation of the `TOTPService`.
struct DefaultTOTPService: TOTPService {
// MARK: Private properties

/// The service used by the application to handle encryption and decryption tasks.
private let clientService: ClientService
/// The service used by the application for sharing data with other apps.
private let pasteboardService: PasteboardService
/// The service used by the application to manage account state.
private let stateService: StateService

// MARK: Init

/// Initializes a `DefaultTOTPService`.
/// - Parameters:
/// - clientService: The service used by the application to handle encryption and decryption tasks.
/// - pasteboardService: The service used by the application for sharing data with other apps.
/// - stateService: The service used by the application to manage account state.
init(
clientService: ClientService,
pasteboardService: PasteboardService,
stateService: StateService
) {
self.clientService = clientService
self.pasteboardService = pasteboardService
self.stateService = stateService
}

// MARK: Methods

func copyTotpIfPossible(cipher: CipherView) async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 I really like pulling this sort of functionality into the TOTPService. I was very confused when I first got into the codebase and a fair number of TOTP-related things were scattered about in different places.

guard let totp = cipher.login?.totp else {
return
}

let disableAutoTotpCopy = try await stateService.getDisableAutoTotpCopy()
guard !disableAutoTotpCopy else {
return
}

let accountHasPremium = try await stateService.doesActiveAccountHavePremium()
guard cipher.organizationUseTotp || accountHasPremium else {
return
}

let codeModel = try await clientService.vault().generateTOTPCode(for: totp, date: nil)
pasteboardService.copy(codeModel.code)
}

/// Retrieves the TOTP configuration for a given key.
///
/// - Parameter key: A string representing the TOTP key.
Expand Down
Loading
Loading
0