Add generic passwork checker before sending transaction #867

ghost · 2018-07-30T13:59:05Z

Referencing #830, I will create new coordinator for this one, and reuse the LockEnterPasscodeViewController.
I would like to get some info before finishing:

Should we include biometric confirm, as well as passcode?
Should we include 'Cancel' button for passcode screen? If so, should we store the current number of unsuccessful attempts?
If the number of unsuccessful attempts is exceeded, should we log the user out?

vikmeup · 2018-07-30T14:38:40Z

@BloodyPixy I have added a new coordinator, forgot that name, take a look in the Lock coordinators. I haven’t done much tho.

vikmeup · 2018-07-30T16:24:01Z

Take a look at: https://github.com/TrustWallet/trust-wallet-ios/blob/55c1d9e48e3c9f5809fb2f1bd60c58cbac511e8b/Trust/Lock/Coordinators/AuthenticateUserCoordinator.swift

this has to be separated an tested.
In order to make this work you need to add an option settings to ask for confirmation.
Make sure use correct view model with messanging.

vikmeup · 2018-07-31T07:53:17Z

@BloodyPixy You need to pass view model that will describe actions!

…passcode.

…touch.id

Trust/Lock/ViewModels/LockEnterPasscodeViewModel.swift

vikmeup · 2018-08-01T16:54:01Z

Trust/Transfer/ViewControllers/ConfirmPaymentViewController.swift


-        let transaction = configurator.signTransaction
+        if needsPasscodeCheck {
+            delegate?.confirmPaymentControllerNeedsAuthentication(self)


is there a way to check this on coordinator level?

…ansactions * master: Add option to skip import main wallet Disable AuthenticateUserCoordinatorTests Version bump Use valueToSend() Add RPCServer as new parameter of the NonFungibleTokenObject. So now we could open nftokens. (#872) Fix an issue with adding extra ether to each transaction Enable deletion of the main wallet Average USD field update on QR code scanning. (#861)

vikmeup

@BloodyPixy can you add tests?

vikmeup · 2018-08-02T16:55:32Z

Trust/Settings/ViewControllers/SettingsViewController.swift

+            $0.title = self?.viewModel.verifyTransactionsWithPasscodeTitle
+            $0.value = self?.viewModel.isPasscodeTransactionLockEnabled
+            $0.hidden = Condition.function([Values.passcodeRow], { form in
+                return !((form.rowBy(tag: Values.passcodeRow) as? SwitchRow)?.value ?? false)


Do something along this:

var privateKeyRow: TextAreaRow? { return form.rowBy(tag: Values.privateKey) }

you mean for passcodeRow, and use it instead of this
!((form.rowBy(tag: Values.passcodeRow) as? SwitchRow)?.value ?? false)
like this
!(self?.passcodeRow?.value ?? false),
did I get you right?

vikmeup · 2018-08-02T16:56:32Z

Trust/Settings/ViewControllers/SettingsViewController.swift

@@ -328,6 +349,12 @@ final class SettingsViewController: FormViewController, Coordinator {
        self.autoLockRow.reload()
    }

+    private func updatePasscodeTransactionLockRow(enabled: Bool) {
+        self.viewModel.isPasscodeTransactionLockEnabled = enabled


do not use self

vikmeup · 2018-08-02T16:57:21Z

Trust/Settings/ViewModels/SettingsViewModel.swift

+    }
+
+    var verifyTransactionsWithPasscodeTitle: String {
+        return NSLocalizedString("settings.lockTransactions.label.title", value: "Authenticate Transactions", comment: "")


vikmeup · 2018-08-02T17:04:43Z

Trust/Transfer/Coordinators/ConfirmCoordinator.swift

+
+extension ConfirmCoordinator: ConfirmPaymentAuthenticationDelegate {
+    func confirmPaymentControllerNeedsAuthentication(_ controller: ConfirmPaymentViewController) {
+        guard let needsPasscodeCheck = PreferencesController()


This means you can modify user defaults to enable this feature?

It needs to be stored on keychain level then

…odeRow.

…ansactions * master: Add new line with wallet name on Transaction Confirmation (#866) Mark wallet as main when imported Add link to help center (#876) # Conflicts: # Trust.xcodeproj/project.pbxproj

ghost · 2018-08-03T07:29:42Z

I decided to put it inside of the lock. Seems logical to me, as we use Lock for keychain work with a passcode, and transaction authorization is directly connected to it. Good thing would be to have a new icon for this option in settings, as for now it uses same icon as Passcode app lock.

It works like this:

User enables app passcode protection and sets the passcode
New line in table appears with a switch to authorize transactions
User can turn it 'on'
If user turns App passcode lock off, transaction authorization will turn itself off as well.
When user performs a transaction with Authorization turned on, app checks for this value, and presents passcode protection view.
If the passcode is correct, transaction is executed.

…ansactions * master: Move reload into separate function Crash: Signing on watch address via browser #885 fix crash when signing on watch address via browser #885 (#887) Update localized string and use R Move into separate function Update Analitics.swift (#882) Link "Settings → Telegram Group" should open channel according to user's locale (#879) Improve currentWalletDescriptionString Move Help center higher in order Version Bump

vikmeup · 2018-08-06T19:25:20Z

Let's hold off on this one. It still requires UX/UI work to figure this out.

Add generic passwork checker before sending transaction

55c1d9e

Use AuthenticateUserCoordinator

644a594

taraspasichnyk added 5 commits July 31, 2018 12:01

Add properly in user defaults. Implemented functionality of checking …

a323fef

…passcode.

Add protocol for view model in LockEnterPasscodeViewController

863d444

Add new files in folder hierarchy

3f990bc

Add localized string for lock.authenticate.enter.passcode.view.model.…

ea7bf52

…touch.id

Add localized strings for settings.lockTransactions.label.title

947a158

vikmeup reviewed Aug 1, 2018

View reviewed changes

taraspasichnyk added 6 commits August 2, 2018 11:16

Check if user needs to confirm with a password on Coordinator level

86c17c3

Use R.string.localizable. Fix issue with Localizable files

15f678b

Change LockCreatePasscodeViewModel to use R.string.localizable

ef83945

Remove duplication

f1d62af

Passcode protect transaction option only visible if passcode lock is set

bef4c80

vikmeup reviewed Aug 2, 2018

View reviewed changes

taraspasichnyk added 3 commits August 2, 2018 21:15

Fix localizable string. Remove 'self'. Add general reference to passc…

9faf26b

…odeRow.

Merge branch 'master' into feature/830-require-password-on-sending-tr…

18dbc31

…ansactions * master: Add new line with wallet name on Transaction Confirmation (#866) Mark wallet as main when imported Add link to help center (#876) # Conflicts: # Trust.xcodeproj/project.pbxproj

Change user defaults to keychain

d453f52

taraspasichnyk added 3 commits August 3, 2018 10:33

Removed localized strings

14121c6

Updated fake lock protocol

ee2dc8a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic passwork checker before sending transaction #867

Add generic passwork checker before sending transaction #867

ghost commented Jul 30, 2018

vikmeup commented Jul 30, 2018

vikmeup commented Jul 30, 2018

vikmeup commented Jul 31, 2018

vikmeup Aug 1, 2018

ghost Aug 2, 2018

vikmeup left a comment

vikmeup Aug 2, 2018

ghost Aug 2, 2018

vikmeup Aug 2, 2018

ghost Aug 3, 2018

vikmeup Aug 2, 2018

ghost Aug 3, 2018

vikmeup Aug 2, 2018

ghost Aug 3, 2018

ghost commented Aug 3, 2018

vikmeup commented Aug 6, 2018

Add generic passwork checker before sending transaction #867

Are you sure you want to change the base?

Add generic passwork checker before sending transaction #867

Conversation

ghost commented Jul 30, 2018

vikmeup commented Jul 30, 2018

vikmeup commented Jul 30, 2018

vikmeup commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikmeup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 3, 2018

vikmeup commented Aug 6, 2018