This repository was archived by the owner on Jun 2, 2019. It is now read-only.

Add generic passwork checker before sending transaction #867

Open

ghost wants to merge 19 commits into master

from feature/830-require-password-on-sending-transactions

Open

Add generic passwork checker before sending transaction #867

ghost wants to merge 19 commits into master from feature/830-require-password-on-sending-transactions

Conversation

ghost

Copy link

@ghost ghost commented Jul 30, 2018

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?

@taraspasichnyk


 Add generic passwork checker before sending transaction

55c1d9e

@vikmeup

Copy link

Contributor

vikmeup commented Jul 30, 2018

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

@vikmeup

Copy link

Contributor

vikmeup commented Jul 30, 2018

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.

@taraspasichnyk


 Use AuthenticateUserCoordinator

644a594

@vikmeup

Copy link

Contributor

vikmeup commented Jul 31, 2018

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

taraspasichnyk added 5 commits

July 31, 2018 12:01

@taraspasichnyk


 Add properly in user defaults. Implemented functionality of checking ...

a323fef

...passcode.

@taraspasichnyk


 Add protocol for view model in LockEnterPasscodeViewController

863d444

@taraspasichnyk


 Add new files in folder hierarchy

3f990bc

@taraspasichnyk


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

ea7bf52

...touch.id

@taraspasichnyk


 Add localized strings for settings.lockTransactions.label.title

947a158

vikmeup

vikmeup reviewed

Aug 1, 2018

View reviewed changes

Trust/Lock/ViewModels/LockEnterPasscodeViewModel.swift Show resolved Hide resolved

Trust/Transfer/ViewControllers/ConfirmPaymentViewController.swift Outdated

let transaction = configurator.signTransaction

if needsPasscodeCheck {

delegate?.confirmPaymentControllerNeedsAuthentication(self)

Copy link

Contributor

Choose a reason for hiding this comment

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

is there a way to check this on coordinator level?

Copy link

Author

Choose a reason for hiding this comment

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

Done

taraspasichnyk added 6 commits

August 2, 2018 11:16

@taraspasichnyk


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

...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)

@taraspasichnyk


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

86c17c3

@taraspasichnyk


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

15f678b

@taraspasichnyk


 Change LockCreatePasscodeViewModel to use R.string.localizable

ef83945

@taraspasichnyk


 Remove duplication

f1d62af

@taraspasichnyk


 Passcode protect transaction option only visible if passcode lock is set

bef4c80

vikmeup

vikmeup reviewed

Aug 2, 2018

View reviewed changes

Copy link

Contributor

@vikmeup vikmeup left a comment

Choose a reason for hiding this comment

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

@BloodyPixy can you add tests?

vikmeup

vikmeup reviewed

Aug 2, 2018

View reviewed changes

Trust/Settings/ViewControllers/SettingsViewController.swift Outdated

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)

Copy link

Contributor

Choose a reason for hiding this comment

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

Do something along this:

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

Copy link

Author

Choose a reason for hiding this comment

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

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?

Trust/Settings/ViewControllers/SettingsViewController.swift Outdated

}

private func updatePasscodeTransactionLockRow(enabled: Bool) {

self.viewModel.isPasscodeTransactionLockEnabled = enabled

Copy link

Contributor

Choose a reason for hiding this comment

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

do not use self

Copy link

Author

Choose a reason for hiding this comment

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

Done

Trust/Settings/ViewModels/SettingsViewModel.swift Outdated

}

var verifyTransactionsWithPasscodeTitle: String {

return NSLocalizedString("settings.lockTransactions.label.title", value: "Authenticate Transactions", comment: "")

Copy link

Contributor

Choose a reason for hiding this comment

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

Use R

Copy link

Author

Choose a reason for hiding this comment

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

Done

Trust/Transfer/Coordinators/ConfirmCoordinator.swift Outdated

extension ConfirmCoordinator: ConfirmPaymentAuthenticationDelegate {

func confirmPaymentControllerNeedsAuthentication(_ controller: ConfirmPaymentViewController) {

guard let needsPasscodeCheck = PreferencesController()

Copy link

Contributor

Choose a reason for hiding this comment

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

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

It needs to be stored on keychain level then

Copy link

Author

Choose a reason for hiding this comment

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

Done

taraspasichnyk added 3 commits

August 2, 2018 21:15

@taraspasichnyk


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

9faf26b

...odeRow.

@taraspasichnyk


 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

@taraspasichnyk


 Change user defaults to keychain

d453f52

@ghost

Copy link

Author

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.

taraspasichnyk added 3 commits

August 3, 2018 10:33

@taraspasichnyk


 Removed localized strings

14121c6

@taraspasichnyk


 Updated fake lock protocol

ee2dc8a

@taraspasichnyk


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

b5c6cde

...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

Copy link

Contributor

vikmeup commented Aug 6, 2018

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

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

Uh oh!

Conversation

@ghost ghost commented Jul 30, 2018

Uh oh!

vikmeup commented Jul 30, 2018

Uh oh!

vikmeup commented Jul 30, 2018

Uh oh!

vikmeup commented Jul 31, 2018

Uh oh!

Uh oh!

@vikmeup vikmeup Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

@ghost ghost Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

@vikmeup vikmeup left a comment

Choose a reason for hiding this comment

Uh oh!

@vikmeup vikmeup Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

@ghost ghost Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

@vikmeup vikmeup Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

@ghost ghost Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

@vikmeup vikmeup Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

@ghost ghost Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

@vikmeup vikmeup Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

@ghost ghost Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 3, 2018

Uh oh!

vikmeup commented Aug 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants