near / near-wallet

Web wallet for NEAR Protocol which stores keys in browser's localStorage
https://wallet.near.org
MIT License
220 stars 176 forks source link

nearnames login flow leaves a few full access keys on the accoun #1490

Open evgenykuzyakov opened 3 years ago

evgenykuzyakov commented 3 years ago

Problem After using nearnames.com to claim the account and move it to a wallet, the wallet doesn't secure the account by removing the previous access key. If you look at the account social.near you can see there is 3 full-access keys right now, but I would prefer there were only one. There is no way to remove unnecessary access keys from the wallet UI. A user doesn't know there are more access keys on their account.

Expected Behavior Either be able to remove all other access keys and replace it with a new one. Or remove the old access keys when moving the account to a NEAR wallet using recovery.

Steps to reproduce

  1. Get a claim link from nearnames.com
  2. Claim the account
  3. Recover this account in the wallet using the seed phrase from the nearnames.com
  4. Secure the account in the wallet, by using Enable seed phrase option.
  5. You now have 3 full access keys on the account. You can't see them in the UI.

Keys

near keys social.near                                                                                                                        
Keys for account social.near
[
  {
    public_key: 'ed25519:6bdxaKXCKpAvVWK1L4HeDLg4z4YXdANoZc6f8ERzHCXx',
    access_key: { nonce: 1, permission: 'FullAccess' }
  },
  {
    public_key: 'ed25519:Chq1MnjG3XTKYxcfekev8DE45YfW2f6gQtxBXcvyaWg5',
    access_key: { nonce: 0, permission: 'FullAccess' }
  },
  {
    public_key: 'ed25519:E9vncsJMBs9GjQ7Bm1KBKQ2wVNvHoRhiGGDenvXJ7Auy',
    access_key: { nonce: 1, permission: 'FullAccess' }
  },
  [length]: 3
]
stefanopepe commented 3 years ago

Can it be done from nearnames application, using something like a batch transaction?

evgenykuzyakov commented 3 years ago

It's more general problem. The wallet UI doesn't display all full-access keys. It would be nice to identify the main key that the wallet uses and other keys, that the wallet UI doesn't know about.

marcinbodnar commented 3 years ago

We hide the Full Access Keys page from Wallet UI, there is no link to it. The best way to solve it would be to remove unused access keys while moving to Wallet, like proposed.

stefanopepe commented 3 years ago

Also part of the #1584 effort to improve wallet keys management

stefanopepe commented 3 years ago

@mattlockyer do you know if this one is still valid?

mattlockyer commented 3 years ago

@jimmy3dita when you come from nearnames you have seed phrase, you can log into wallet but you see no recovery options. Typically user adds another recovery option at this point.

Not the end of the world if user has 1 more full access key and thus 1 more backup option beyond the wallet.

Not sure how wallet team wants to handle this.

It should look at recovery options, see that user has this full access key that is NOT a recovery option and communicate accordingly like in the ledger situation.

"It appears you have full access keys that are not official NEAR Wallet recovery methods. Did you use another method to create your wallet such as "nearnames.com"? Click here to remove these keys -> BUTTON."

stefanopepe commented 3 years ago

If I understand it right, the private key generated by nearnames is shared in plain text, and the receiver has no control over its secure transmission (e.g. via Whatsapp, SMS or email - via copy/paste). I was able to clean-up by using Ledger, as it deleted the seed and all previous keys.

Ideally, the user should go through a similar "cleanup" process that removes this potentially compromised key, introducing a new one they generated on a trusted system.

This may benefit from a "key rotation feature" for the wallet, which might do as you propose: check if there are full-access keys that are not recognized as recovery keys, and suggest the user replace some keys and secure the funds again.

@vgrichina @Patrick1904 how you guys feel about this solution?