Skip to content

Commit b2fe8b5

Browse files
matt-livefrontmatt-livefront
authored
PM-9660: Improve autofill extension flow with never lock (#757)
1 parent ac5dcfa commit b2fe8b5

File tree

5 files changed

+119
-26
lines changed

5 files changed

+119
-26
lines changed

‎BitwardenShared/Core/Autofill/Services/AutofillCredentialService.swift

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import AuthenticationServices
22
import BitwardenSdk
33
import OSLog
44

5+
// swiftlint:disable file_length
6+
57
/// A delegate to handle autofill credential service operations.
68
protocol AutofillCredentialServiceDelegate: AnyObject {
79
/// Attempts to unlock the user's vault with the stored neverlock key
@@ -16,12 +18,17 @@ protocol AutofillCredentialService: AnyObject {
1618
///
1719
/// - Parameters:
1820
/// - id: The identifier of the user-requested credential to return.
21+
/// - autofillCredentialServiceDelegate: Delegate for autofill credential operations.
1922
/// - repromptPasswordValidated: `true` if master password reprompt was required for the
2023
/// cipher and the user's master password was validated.
2124
/// - Returns: A `ASPasswordCredential` that matches the user-requested credential which can be
2225
/// used for autofill.
2326
///
24-
func provideCredential(for id: String, repromptPasswordValidated: Bool) async throws -> ASPasswordCredential
27+
func provideCredential(
28+
for id: String,
29+
autofillCredentialServiceDelegate: AutofillCredentialServiceDelegate,
30+
repromptPasswordValidated: Bool
31+
) async throws -> ASPasswordCredential
2532

2633
/// Provides a Fido2 credential for a passkey request
2734
/// - Parameters:
@@ -203,10 +210,28 @@ class DefaultAutofillCredentialService {
203210
errorReporter.log(error: error)
204211
}
205212
}
213+
214+
/// Attempts to unlock the user's vault if it can be done without user interaction (e.g. if
215+
/// the user uses never lock).
216+
///
217+
/// - Parameter delegate: The delegate used for autofill credential operations.
218+
///
219+
private func tryUnlockVaultWithoutUserInteraction(delegate: AutofillCredentialServiceDelegate) async throws {
220+
let userId = try await stateService.getActiveAccountId()
221+
let isLocked = vaultTimeoutService.isLocked(userId: userId)
222+
let vaultTimeout = try? await vaultTimeoutService.sessionTimeoutValue(userId: nil)
223+
guard vaultTimeout == .never, isLocked else { return }
224+
try await delegate.unlockVaultWithNeverlockKey()
225+
}
206226
}
207227

208228
extension DefaultAutofillCredentialService: AutofillCredentialService {
209-
func provideCredential(for id: String, repromptPasswordValidated: Bool) async throws -> ASPasswordCredential {
229+
func provideCredential(
230+
for id: String,
231+
autofillCredentialServiceDelegate: AutofillCredentialServiceDelegate,
232+
repromptPasswordValidated: Bool
233+
) async throws -> ASPasswordCredential {
234+
try await tryUnlockVaultWithoutUserInteraction(delegate: autofillCredentialServiceDelegate)
210235
guard try await !vaultTimeoutService.isLocked(userId: stateService.getActiveAccountId()) else {
211236
throw ASExtensionError(.userInteractionRequired)
212237
}
@@ -255,18 +280,8 @@ extension DefaultAutofillCredentialService: AutofillCredentialService {
255280
throw AppProcessorError.invalidOperation
256281
}
257282

258-
let userId = try await stateService.getActiveAccountId()
259-
let isLocked = vaultTimeoutService.isLocked(userId: userId)
260-
let vaultTimeout = try? await vaultTimeoutService.sessionTimeoutValue(userId: nil)
261-
262-
switch (vaultTimeout, isLocked) {
263-
case (.never, true):
264-
// If the user has enabled Never Lock, but the vault is locked,
265-
// unlock the vault before continuing.
266-
try await autofillCredentialServiceDelegate.unlockVaultWithNeverlockKey()
267-
case (_, false):
268-
break
269-
default:
283+
try await tryUnlockVaultWithoutUserInteraction(delegate: autofillCredentialServiceDelegate)
284+
guard try await !vaultTimeoutService.isLocked(userId: stateService.getActiveAccountId()) else {
270285
throw Fido2Error.userInteractionRequired
271286
}
272287

‎BitwardenShared/Core/Autofill/Services/AutofillCredentialServiceTests.swift

Lines changed: 82 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
8282
stateService.activeAccount = .fixture()
8383
vaultTimeoutService.isClientLocked["1"] = false
8484

85-
let credential = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
85+
let credential = try await subject.provideCredential(
86+
for: "1",
87+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
88+
repromptPasswordValidated: false
89+
)
8690

8791
XCTAssertEqual(credential.password, "password123")
8892
XCTAssertEqual(credential.user, "[email protected]")
@@ -97,17 +101,29 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
97101

98102
cipherService.fetchCipherResult = .success(.fixture(type: .identity))
99103
await assertAsyncThrows(error: ASExtensionError(.credentialIdentityNotFound)) {
100-
_ = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
104+
_ = try await subject.provideCredential(
105+
for: "1",
106+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
107+
repromptPasswordValidated: false
108+
)
101109
}
102110

103111
cipherService.fetchCipherResult = .success(.fixture(login: .fixture(password: nil, username: "user@bitwarden")))
104112
await assertAsyncThrows(error: ASExtensionError(.credentialIdentityNotFound)) {
105-
_ = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
113+
_ = try await subject.provideCredential(
114+
for: "1",
115+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
116+
repromptPasswordValidated: false
117+
)
106118
}
107119

108120
cipherService.fetchCipherResult = .success(.fixture(login: .fixture(password: "test", username: nil)))
109121
await assertAsyncThrows(error: ASExtensionError(.credentialIdentityNotFound)) {
110-
_ = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
122+
_ = try await subject.provideCredential(
123+
for: "1",
124+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
125+
repromptPasswordValidated: false
126+
)
111127
}
112128
}
113129

@@ -117,10 +133,38 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
117133
vaultTimeoutService.isClientLocked["1"] = false
118134

119135
await assertAsyncThrows(error: ASExtensionError(.credentialIdentityNotFound)) {
120-
_ = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
136+
_ = try await subject.provideCredential(
137+
for: "1",
138+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
139+
repromptPasswordValidated: false
140+
)
121141
}
122142
}
123143

144+
/// `provideCredential(for:)` unlocks the user's vault if they use never lock.
145+
func test_provideCredential_neverLock() async throws {
146+
autofillCredentialServiceDelegate.unlockVaultWithNaverlockHandler = { [weak self] in
147+
self?.vaultTimeoutService.isClientLocked["1"] = false
148+
}
149+
cipherService.fetchCipherResult = .success(
150+
.fixture(login: .fixture(password: "password123", username: "[email protected]"))
151+
)
152+
stateService.activeAccount = .fixture()
153+
vaultTimeoutService.isClientLocked["1"] = true
154+
vaultTimeoutService.vaultTimeout["1"] = .never
155+
156+
let credential = try await subject.provideCredential(
157+
for: "1",
158+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
159+
repromptPasswordValidated: false
160+
)
161+
162+
XCTAssertTrue(autofillCredentialServiceDelegate.unlockVaultWithNeverlockKeyCalled)
163+
XCTAssertEqual(credential.password, "password123")
164+
XCTAssertEqual(credential.user, "[email protected]")
165+
XCTAssertNil(pasteboardService.copiedString)
166+
}
167+
124168
/// `provideCredential(for:)` throws an error if reprompt is required.
125169
func test_provideCredential_repromptRequired() async throws {
126170
stateService.activeAccount = .fixture()
@@ -136,7 +180,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
136180
)
137181
)
138182
await assertAsyncThrows(error: ASExtensionError(.userInteractionRequired)) {
139-
_ = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
183+
_ = try await subject.provideCredential(
184+
for: "1",
185+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
186+
repromptPasswordValidated: false
187+
)
140188
}
141189
}
142190

@@ -152,7 +200,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
152200
stateService.activeAccount = .fixture()
153201
vaultTimeoutService.isClientLocked["1"] = false
154202

155-
let credential = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
203+
let credential = try await subject.provideCredential(
204+
for: "1",
205+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
206+
repromptPasswordValidated: false
207+
)
156208

157209
XCTAssertEqual(credential.password, "password123")
158210
XCTAssertEqual(credential.user, "[email protected]")
@@ -173,7 +225,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
173225
stateService.disableAutoTotpCopyByUserId["1"] = true
174226
vaultTimeoutService.isClientLocked["1"] = false
175227

176-
let credential = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
228+
let credential = try await subject.provideCredential(
229+
for: "1",
230+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
231+
repromptPasswordValidated: false
232+
)
177233

178234
XCTAssertEqual(credential.password, "password123")
179235
XCTAssertEqual(credential.user, "[email protected]")
@@ -193,7 +249,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
193249
stateService.doesActiveAccountHavePremiumResult = .success(false)
194250
vaultTimeoutService.isClientLocked["1"] = false
195251

196-
let credential = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
252+
let credential = try await subject.provideCredential(
253+
for: "1",
254+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
255+
repromptPasswordValidated: false
256+
)
197257

198258
XCTAssertEqual(credential.password, "password123")
199259
XCTAssertEqual(credential.user, "[email protected]")
@@ -217,7 +277,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
217277
stateService.doesActiveAccountHavePremiumResult = .success(false)
218278
vaultTimeoutService.isClientLocked["1"] = false
219279

220-
let credential = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
280+
let credential = try await subject.provideCredential(
281+
for: "1",
282+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
283+
repromptPasswordValidated: false
284+
)
221285

222286
XCTAssertEqual(credential.password, "password123")
223287
XCTAssertEqual(credential.user, "[email protected]")
@@ -230,7 +294,11 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
230294
vaultTimeoutService.isClientLocked["1"] = true
231295

232296
await assertAsyncThrows(error: ASExtensionError(.userInteractionRequired)) {
233-
_ = try await subject.provideCredential(for: "1", repromptPasswordValidated: false)
297+
_ = try await subject.provideCredential(
298+
for: "1",
299+
autofillCredentialServiceDelegate: autofillCredentialServiceDelegate,
300+
repromptPasswordValidated: false
301+
)
234302
}
235303
}
236304

@@ -279,6 +347,9 @@ class AutofillCredentialServiceTests: BitwardenTestCase { // swiftlint:disable:t
279347
/// succeeds when unlocking with never key.
280348
@available(iOS 17.0, *)
281349
func test_provideFido2Credential_succeedsWithUnlockingNeverKey() async throws {
350+
autofillCredentialServiceDelegate.unlockVaultWithNaverlockHandler = { [weak self] in
351+
self?.vaultTimeoutService.isClientLocked["1"] = false
352+
}
282353
stateService.activeAccount = .fixture()
283354
vaultTimeoutService.isClientLocked["1"] = true
284355
vaultTimeoutService.vaultTimeout["1"] = .never

‎BitwardenShared/Core/Autofill/Services/TestHelpers/MockAutofillCredentialService.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ class MockAutofillCredentialService: AutofillCredentialService {
1010
var provideCredentialError: Error?
1111
var provideFido2CredentialResult: Result<PasskeyAssertionCredential, Error> = .failure(BitwardenTestError.example)
1212

13-
func provideCredential(for id: String, repromptPasswordValidated: Bool) async throws -> ASPasswordCredential {
13+
func provideCredential(
14+
for id: String,
15+
autofillCredentialServiceDelegate: AutofillCredentialServiceDelegate,
16+
repromptPasswordValidated: Bool
17+
) async throws -> ASPasswordCredential {
1418
guard let provideCredentialPasswordCredential else {
1519
throw provideCredentialError ?? ASExtensionError(.failed)
1620
}

‎BitwardenShared/Core/Autofill/Services/TestHelpers/MockAutofillCredentialServiceDelegate.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
class MockAutofillCredentialServiceDelegate: AutofillCredentialServiceDelegate {
44
var unlockVaultWithNeverlockKeyCalled = false
55
var unlockVaultWithNeverlockKeyError: Error?
6+
var unlockVaultWithNaverlockHandler: (() -> Void)?
67

78
func unlockVaultWithNeverlockKey() async throws {
89
unlockVaultWithNeverlockKeyCalled = true
10+
unlockVaultWithNaverlockHandler?()
911
if let unlockVaultWithNeverlockKeyError {
1012
throw unlockVaultWithNeverlockKeyError
1113
}

‎BitwardenShared/UI/Platform/Application/AppProcessor.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,7 @@ public class AppProcessor {
155155
) async throws -> ASPasswordCredential {
156156
try await services.autofillCredentialService.provideCredential(
157157
for: id,
158+
autofillCredentialServiceDelegate: self,
158159
repromptPasswordValidated: repromptPasswordValidated
159160
)
160161
}

0 commit comments

Comments
 (0)