Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

Commit

Permalink
fix autofill issues with rust logins (#961)
Browse files Browse the repository at this point in the history 
* initial fix

* update specs, fix locking

* back to async

* bind to main thread

* flip case for initialization
  • Loading branch information
@sashei
sashei authored May 10, 2019
1 parent e3cc957 commit 8f19a37
Show file tree
Hide file tree
Showing 9 changed files with 61 additions and 67 deletions.
16 changes: 8 additions & 8 deletions CredentialProvider/Presenter/CredentialItemListPresenter.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,17 @@ class ItemListPresenter: BaseItemListPresenter {
view.dismissKeyboard()
}

self.dataStore.get(id)
.take(1)
.subscribe(onNext: { login in
target.dataStore.get(id)
.map { login -> CredentialStatusAction in
if let login = login {
target.dispatcher.dispatch(action: CredentialStatusAction.loginSelected(login: login, relock: false))
return CredentialStatusAction.loginSelected(login: login, relock: false)
} else {
target.dispatcher.dispatch(action: CredentialStatusAction.userCanceled)
return CredentialStatusAction.userCanceled
}
})
.disposed(by: self.disposeBag)
}.asObserver()
}
.subscribe(onNext: { target.dispatcher.dispatch(action: $0) })
.disposed(by: target.disposeBag)
}.asObserver()
}

override var helpTextItems: [LoginListCellConfiguration] { return [LoginListCellConfiguration.SelectAPasswordHelpText] }
Expand Down
12 changes: 4 additions & 8 deletions CredentialProvider/Presenter/CredentialProviderPresenter.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,11 @@ class CredentialProviderPresenter {
})
.disposed(by: self.disposeBag)

self.dispatcher.dispatch(action: LifecycleAction.foreground)
self.dispatcher.dispatch(action: DataStoreAction.unlock)
self.startTelemetry()
}

func extensionConfigurationRequested() {
self.dispatcher.dispatch(action: LifecycleAction.foreground)

self.dataStore.locked
.bind { [weak self] locked in
if locked {
Expand All @@ -104,8 +101,6 @@ class CredentialProviderPresenter {
}

func credentialProvisionRequested(for credentialIdentity: ASPasswordCredentialIdentity) {
self.dispatcher.dispatch(action: LifecycleAction.foreground)

self.dataStore.locked
.take(1)
.bind { [weak self] locked in
Expand All @@ -123,8 +118,6 @@ class CredentialProviderPresenter {
}

func credentialList(for serviceIdentifiers: [ASCredentialServiceIdentifier]) {
self.dispatcher.dispatch(action: LifecycleAction.foreground)

self.dataStore.locked
.asDriver(onErrorJustReturn: true)
.drive(onNext: { [weak self] locked in
Expand Down Expand Up @@ -154,7 +147,10 @@ extension CredentialProviderPresenter {
return
}

self.dataStore.get(id)
self.dataStore.locked
.filter { !$0 }
.take(1)
.flatMap { _ in self.dataStore.get(id) }
.bind { [weak self] login in
guard let login = login else {
self?.cancelWith(.credentialIdentityNotFound)
Expand Down
25 changes: 16 additions & 9 deletions Shared/Store/BaseDataStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,10 @@ class BaseDataStore {
self.lifecycleStore.lifecycleEvents
.filter { $0 == .foreground }
.subscribe(onNext: { [weak self] _ in
self?.initializeLoginsStorage()
guard let _ = self?.loginsStorage else {
self?.initializeLoginsStorage()
return
}
})
.disposed(by: self.disposeBag)

Expand All @@ -187,12 +190,18 @@ class BaseDataStore {
}

public func get(_ id: String) -> Observable<LoginRecord?> {
return self.listSubject
.map { items -> LoginRecord? in
return items.filter { item in
return item.id == id
}.first
}
return Observable.create({ [weak self] observer -> Disposable in
self?.queue.async {
do {
let login = try self?.loginsStorage?.get(id: id)
observer.onNext(login)
} catch let error {
observer.onError(error)
}
}

return Disposables.create()
})
}

public func initialized() {
Expand Down Expand Up @@ -258,7 +267,6 @@ extension BaseDataStore {
extension BaseDataStore {
private func setupAutolock() {
self.lifecycleStore.lifecycleEvents
.distinctUntilChanged()
.filter { $0 == .background }
.flatMap { _ in self.storageState }
.take(1)
Expand All @@ -270,7 +278,6 @@ extension BaseDataStore {
.disposed(by: disposeBag)

self.lifecycleStore.lifecycleEvents
.distinctUntilChanged()
.filter { $0 == .foreground }
.flatMap { _ in self.storageState }
.take(1)
Expand Down
3 changes: 1 addition & 2 deletions Shared/Store/BaseItemDetailStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,13 @@ class BaseItemDetailStore {
internal var dispatcher: Dispatcher
internal var disposeBag = DisposeBag()

internal var _itemDetailId = ReplaySubject<String>.create(bufferSize: 1)
internal var _itemDetailId = BehaviorRelay<String>.init(value: "")

lazy private(set) var itemDetailId: Observable<String> = {
return self._itemDetailId.asObservable()
}()

init(dispatcher: Dispatcher = Dispatcher.shared) {
self.dispatcher = dispatcher
self._itemDetailId.onNext("")
}
}
5 changes: 1 addition & 4 deletions lockbox-ios/Presenter/ItemDetailPresenter.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ class ItemDetailPresenter {
target.itemDetailStore.itemDetailId
.take(1)
.flatMap { target.dataStore.get($0) }
// The first `take(1)` call does not push a completion after it emits, so a second one is necessary here
// to ensure that subsequent updates to the item do not result in subsequent dispacthed actions.
.take(1)
.map { item -> [Action] in
var actions: [Action] = []
if copyableFields.contains(value) {
Expand All @@ -64,6 +61,7 @@ class ItemDetailPresenter {
}
return actions
}
.observeOn(MainScheduler.instance)
.subscribe(onNext: { actions in
for action in actions {
target.dispatcher.dispatch(action: action)
Expand Down Expand Up @@ -153,7 +151,6 @@ class ItemDetailPresenter {
self.itemDetailStore.itemDetailId
.take(1)
.flatMap { self.dataStore.get($0) }
.take(1)
.flatMap { item -> Observable<[Action]> in
var actions: [Action] = []
if let item = item {
Expand Down
4 changes: 2 additions & 2 deletions lockbox-ios/Store/ItemDetailStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class ItemDetailStore: BaseItemDetailStore {
.subscribe(onNext: { (route) in
switch route {
case .detail(let itemId):
self._itemDetailId.onNext(itemId)
self._itemDetailId.accept(itemId)
case .list:
break
}
Expand Down Expand Up @@ -82,7 +82,7 @@ class ItemDetailStore: BaseItemDetailStore {
private func showFirstLogin(_ login: LoginRecord?) {
if let login = login {
runOnMainThread {
self._itemDetailId.onNext(login.id)
self._itemDetailId.accept(login.id)
}
}
}
Expand Down
16 changes: 3 additions & 13 deletions lockbox-iosTests/BaseDataStoreSpec.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@ class BaseDataStoreSpec: QuickSpec {
self.dataStoreSupport.clearInvocations()
self.lifecycleStore.fakeCycle.onNext(.foreground)
_ = try! self.subject.storageState.toBlocking().first()
expect(self.dataStoreSupport.createArgument).notTo(beNil())
expect(self.listObserver.events.last!.value.element!).to(beEmpty())
}
}
Expand Down Expand Up @@ -306,6 +305,7 @@ class BaseDataStoreSpec: QuickSpec {
}

it("locks") {
_ = try! self.subject.storageState.toBlocking().first()
let state = try! self.subject.storageState.toBlocking().first()
expect(self.loginsStorage.ensureLockedCalled).to(beTrue())
expect(state).to(equal(LoginStoreState.Locked))
Expand Down Expand Up @@ -334,6 +334,7 @@ class BaseDataStoreSpec: QuickSpec {

it("backdates the next lock time and locks the db") {
expect(self.autoLockSupport.backdateCalled).to(beTrue())
_ = try! self.subject.storageState.toBlocking().first()
let state = try! self.subject.storageState.toBlocking().first()
expect(self.loginsStorage.ensureLockedCalled).to(beTrue())
expect(state).to(equal(LoginStoreState.Locked))
Expand Down Expand Up @@ -376,8 +377,8 @@ class BaseDataStoreSpec: QuickSpec {
}

it("syncs + pushes syncing followed by synced") {
_ = try! self.subject.syncState.toBlocking().first()
expect(self.loginsStorage.syncArgument).notTo(beNil())
let _ = try! self.subject.syncState.toBlocking().first()
let syncStates: [SyncState] = self.syncObserver.events.map {
$0.value.element!
}
Expand Down Expand Up @@ -435,17 +436,6 @@ class BaseDataStoreSpec: QuickSpec {
expect(self.loginsStorage.closeCalled).to(beTrue())
}
}

describe("foreground events") {
beforeEach {
self.dataStoreSupport.clearInvocations()
self.lifecycleStore.fakeCycle.onNext(.foreground)
}

it("re-inits the datastore") {
expect(self.dataStoreSupport.createArgument).notTo(beNil())
}
}
}
}

Expand Down
45 changes: 25 additions & 20 deletions lockbox-iosTests/CredentialProviderPresenterSpec.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class CredentialProviderPresenterSpec: QuickSpec {
}

class FakeDataStore: DataStore {
let lockedStub = PublishSubject<Bool>()
let lockedStub = ReplaySubject<Bool>.create(bufferSize: 1)
let getStub = PublishSubject<LoginRecord?>()

var getGuid: String?
Expand Down Expand Up @@ -179,8 +179,7 @@ class CredentialProviderPresenterSpec: QuickSpec {
self.subject.extensionConfigurationRequested()
}

it("foregrounds the datastore and tells the view to display the welcome screen") {
expect(self.dispatcher.actionArguments.popLast() as? LifecycleAction).to(equal(LifecycleAction.foreground))
it("tells the view to display the welcome screen") {
expect(self.view.displayWelcomeCalled).to(beTrue())
}
}
Expand Down Expand Up @@ -231,32 +230,38 @@ class CredentialProviderPresenterSpec: QuickSpec {
it("unlocks the datastore") {
expect(self.dispatcher.actionArguments.popLast()! as? DataStoreAction).to(equal(DataStoreAction.unlock))
}

it("requests the login from the datastore") {
expect(self.dataStore.getGuid).to(equal(passwordIdentity.recordIdentifier))
}

describe("when the login is nil") {
describe("after unlocking") {
beforeEach {
self.dataStore.getStub.onNext(nil)
self.dataStore.lockedStub.onNext(false)
}

it("cancels the request with notFound") {
expect(self.view.fakeExtensionContext.error).notTo(beNil())
expect((self.view.fakeExtensionContext.error! as NSError).code).to(equal(ASExtensionError.credentialIdentityNotFound.rawValue))
it("requests the login from the datastore") {
expect(self.dataStore.getGuid).to(equal(passwordIdentity.recordIdentifier))
}
}

describe("when the login is not nil and the password fill completes") {
let login = LoginRecord(fromJSONDict: ["id": guid, "hostname": "http://www.mozilla.com", "username": "dogs@dogs.com", "password": "meow"])
describe("when the login is nil") {
beforeEach {
self.dataStore.getStub.onNext(nil)
}

beforeEach {
self.dataStore.getStub.onNext(login)
it("cancels the request with notFound") {
expect(self.view.fakeExtensionContext.error).notTo(beNil())
expect((self.view.fakeExtensionContext.error! as NSError).code).to(equal(ASExtensionError.credentialIdentityNotFound.rawValue))
}
}

it("dispatches the selected credential action") {
expect(self.dispatcher.actionArguments.popLast()! as? CredentialStatusAction).to(equal(CredentialStatusAction.loginSelected(login: login, relock: true)))
describe("when the login is not nil and the password fill completes") {
let login = LoginRecord(fromJSONDict: ["id": guid, "hostname": "http://www.mozilla.com", "username": "dogs@dogs.com", "password": "meow"])

beforeEach {
self.dataStore.getStub.onNext(login)
}

it("dispatches the selected credential action") {
expect(self.dispatcher.actionArguments.popLast()! as? CredentialStatusAction).to(equal(CredentialStatusAction.loginSelected(login: login, relock: true)))
}
}

}
}

Expand Down
2 changes: 1 addition & 1 deletion lockbox-iosTests/ItemDetailPresenterSpec.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class ItemDetailPresenterSpec: QuickSpec {
}

it("dispatches nothing new") {
expect(self.dispatcher.dispatchActionArgument.count).to(equal(2))
expect(self.dispatcher.dispatchActionArgument.count).to(equal(3))
}
}
}
Expand Down

0 comments on commit 8f19a37

Please sign in to comment.