Skip to content
This repository has been archived by the owner on Oct 4, 2024. It is now read-only.

Feature: Add 2FA Support to CLI #535

Merged
merged 12 commits into from
Oct 12, 2020
Merged

Feature: Add 2FA Support to CLI #535

merged 12 commits into from
Oct 12, 2020

Conversation

mattlockyer
Copy link

WIP can send transfers using 2fa account with --use2fa

TODO

  1. Update convertActions helper to support deployContract
  2. Test more CLI methods e.g. deploy

@mattlockyer
Copy link
Author

WIP UX for send command
image

@ilblackdragon
Copy link
Member

ilblackdragon commented Sep 10, 2020

Is there a way to not need --2fa flag? Ideally we should figure out it from account itself.

Multisig is easily detectable. Not sure if we want to expose CH to tell if this account is managed by it or not - looks a bit like of privacy/attack issue. Right now it's exposed just by the fact that the key is the same, but we would ideally generate new public key per user going forward.

Also Account object should work not just specifically to 2fa but for any multisig account. This probably can even live in near-api-js eventually.

And 2FA should be just extra wrapper over Account that if it's 2fa account - after request was created it will wait for confirmation and query for result after tx confirmed.

@mattlockyer
Copy link
Author

Is there a way to not need --2fa flag? Ideally we should figure out it from account itself.

Yes we can check hashes or use CH

Multisig is easily detectable. Not sure if we want to expose CH to tell if this account is managed by it or not - looks a bit like of privacy/attack issue. Right now it's exposed just by the fact that the key is the same, but we would ideally generate new public key per user going forward.

Each time you login with wallet you get a 2FA key for the CLI. This key is multisig key only. You need to provide signature to access any CH methods that expose recovery / 2FA details so I think it's secure?

Also Account object should work not just specifically to 2fa but for any multisig account. This probably can even live in near-api-js eventually.

Agree with this point here. Will take a bit of time to think about proper abstractions. Appreciate any input here: @vgrichina

And 2FA should be just extra wrapper over Account that if it's 2fa account - after request was created it will wait for confirmation and query for result after tx confirmed.

Sure. Should we continue with extends Account inheritance pattern to wrap functionality of Account for special contract accounts?

@dentesting
Copy link

dentesting commented Sep 25, 2020

Not sure if we want to expose CH to tell if this account is managed by it or not - looks a bit like of privacy/attack issue.

I think by looking at the storage used (2FA vs none-2FA) and keys, one can easily identify its a 2FA account:
Screenshot 2020-09-25 at 11 46 53

@mattlockyer
Copy link
Author

@dentesting thanks for joining this discussion.

We decided to move the mutlisig support to near-api-js:
near/near-api-js#403

There is already a wallet PR that uses this:
near/near-wallet#914

This will allow us to create a simple CLI PR that detects if the account has a multisig contract (using access keys and method names or contract hashes as it's done in the wallet).

ETA for CLI PR - next week

@dentesting
Copy link

dentesting commented Sep 25, 2020

@dentesting thanks for joining this discussion.
ETA for CLI PR - next week

Excellent, I will be happy to test.

@mattlockyer
Copy link
Author

Works with:
near/near-api-js#414

@mattlockyer mattlockyer marked this pull request as ready for review September 29, 2020 14:51
package.json Outdated Show resolved Hide resolved
utils/connect.js Outdated Show resolved Hide resolved
utils/connect.js Outdated
// TODO: Avoid need to wrap in deps
return await nearConnect({ ...options, deps: { keyStore }});
const near = await nearConnect({ ...options, deps: { keyStore }});
near.account = async (accountId) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this logic should be in near-api-js

utils/connect.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
utils/2fa.js Outdated Show resolved Hide resolved
Copy link
Contributor

@vgrichina vgrichina left a comment

Choose a reason for hiding this comment

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

this looks good, just make sure to address comments in near/near-api-js#428 (review)

@mattlockyer mattlockyer requested a review from vgrichina October 10, 2020 15:36
@mattlockyer mattlockyer merged commit 4807fbc into master Oct 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants