Merge pull request #645 from MetaMask/FixPasswording

Fix account unlocking

app/scripts/lib/id-management.js

@@ -1,3 +1,11 @@
+/* ID Management
+ *
+ * This module exists to hold the decrypted credentials for the current session.
+ * It therefore exposes sign methods, because it is able to perform these
+ * with noa dditional authentication, because its very instantiation
+ * means the vault is unlocked.
+ */
+
 const ethUtil = require('ethereumjs-util')
 const Transaction = require('ethereumjs-tx')
 

app/scripts/lib/idStore.js

@@ -3,7 +3,7 @@ const inherits = require('util').inherits
 const async = require('async')
 const ethUtil = require('ethereumjs-util')
 const EthQuery = require('eth-query')
-const LightwalletKeyStore = require('eth-lightwallet').keystore
+const KeyStore = require('eth-lightwallet').keystore
 const clone = require('clone')
 const extend = require('xtend')
 const createId = require('web3-provider-engine/util/random-id')
@@ -50,15 +50,16 @@ IdentityStore.prototype.createNewVault = function (password, entropy, cb) {
   if (serializedKeystore) {
     this.configManager.setData({})
   }
-  this._createIdmgmt(password, null, entropy, (err) => {
+
+  this.purgeCache()
+  this._createVault(password, null, entropy, (err) => {
     if (err) return cb(err)
 
-    this._loadIdentities()
-    this._didUpdate()
     this._autoFaucet()
 
     this.configManager.setShowSeedWords(true)
     var seedWords = this._idmgmt.getSeed()
+
     cb(null, seedWords)
   })
 }
@@ -71,11 +72,12 @@ IdentityStore.prototype.recoverSeed = function (cb) {
 }
 
 IdentityStore.prototype.recoverFromSeed = function (password, seed, cb) {
-  this._createIdmgmt(password, seed, null, (err) => {
+  this.purgeCache()
+
+  this._createVault(password, seed, null, (err) => {
     if (err) return cb(err)
 
     this._loadIdentities()
-    this._didUpdate()
     cb(null, this.getState())
   })
 }
@@ -125,7 +127,7 @@ IdentityStore.prototype.getSelectedAddress = function () {
   return configManager.getSelectedAccount()
 }
 
-IdentityStore.prototype.setSelectedAddress = function (address, cb) {
+IdentityStore.prototype.setSelectedAddressSync = function (address) {
   const configManager = this.configManager
   if (!address) {
     var addresses = this._getAddresses()
@@ -133,7 +135,12 @@ IdentityStore.prototype.setSelectedAddress = function (address, cb) {
   }
 
   configManager.setSelectedAccount(address)
-  if (cb) return cb(null, address)
+  return address
+}
+
+IdentityStore.prototype.setSelectedAddress = function (address, cb) {
+  const resultAddress = this.setSelectedAddressSync(address)
+  if (cb) return cb(null, resultAddress)
 }
 
 IdentityStore.prototype.revealAccount = function (cb) {
@@ -143,6 +150,11 @@ IdentityStore.prototype.revealAccount = function (cb) {
 
   keyStore.setDefaultHdDerivationPath(this.hdPathString)
   keyStore.generateNewAddress(derivedKey, 1)
+  const addresses = keyStore.getAddresses()
+  const address = addresses[ addresses.length - 1 ]
+
+  this._ethStore.addAccount(ethUtil.addHexPrefix(address))
+
   configManager.setWallet(keyStore.serialize())
 
   this._loadIdentities()
@@ -393,7 +405,7 @@ IdentityStore.prototype._loadIdentities = function () {
   var addresses = this._getAddresses()
   addresses.forEach((address, i) => {
     // // add to ethStore
-    this._ethStore.addAccount(address)
+    this._ethStore.addAccount(ethUtil.addHexPrefix(address))
     // add to identities
     const defaultLabel = 'Wallet ' + (i + 1)
     const nickname = configManager.nicknameForWallet(address)
@@ -412,7 +424,6 @@ IdentityStore.prototype.saveAccountLabel = function (account, label, cb) {
   configManager.setNicknameForWallet(account, label)
   this._loadIdentities()
   cb(null, label)
-  this._didUpdate()
 }
 
 // mayBeFauceting
@@ -436,77 +447,87 @@ IdentityStore.prototype._mayBeFauceting = function (i) {
 //
 
 IdentityStore.prototype.tryPassword = function (password, cb) {
-  this._createIdmgmt(password, null, null, cb)
+  var serializedKeystore = this.configManager.getWallet()
-}
+  var keyStore = KeyStore.deserialize(serializedKeystore)
 
-IdentityStore.prototype._createIdmgmt = function (password, seed, entropy, cb) {
+  keyStore.keyFromPassword(password, (err, pwDerivedKey) => {
-  const configManager = this.configManager
-
-  var keyStore = null
-  LightwalletKeyStore.deriveKeyFromPassword(password, (err, derivedKey) => {
     if (err) return cb(err)
-    var serializedKeystore = configManager.getWallet()
 
-    if (seed) {
+    const isCorrect = keyStore.isDerivedKeyCorrect(pwDerivedKey)
-      try {
+    if (!isCorrect) return cb(new Error('Lightwallet - password incorrect'))
-        keyStore = this._restoreFromSeed(password, seed, derivedKey)
-      } catch (e) {
-        return cb(e)
-      }
-
-    // returning user, recovering from storage
-    } else if (serializedKeystore) {
-      keyStore = LightwalletKeyStore.deserialize(serializedKeystore)
-      var isCorrect = keyStore.isDerivedKeyCorrect(derivedKey)
-      if (!isCorrect) return cb(new Error('Lightwallet - password incorrect'))
-
-   // first time here
-    } else {
-      keyStore = this._createFirstWallet(entropy, derivedKey)
-    }
 
     this._keyStore = keyStore
-    this._idmgmt = new IdManagement({
+    this._createIdMgmt(pwDerivedKey)
-      keyStore: keyStore,
-      derivedKey: derivedKey,
-      hdPathSTring: this.hdPathString,
-      configManager: this.configManager,
-    })
-
     cb()
   })
 }
 
-IdentityStore.prototype._restoreFromSeed = function (password, seed, derivedKey) {
+IdentityStore.prototype._createVault = function (password, seedPhrase, entropy, cb) {
-  const configManager = this.configManager
+  const opts = {
-  var keyStore = new LightwalletKeyStore(seed, derivedKey, this.hdPathString)
+    password,
-  keyStore.addHdDerivationPath(this.hdPathString, derivedKey, {curve: 'secp256k1', purpose: 'sign'})
+    hdPathString: this.hdPathString,
-  keyStore.setDefaultHdDerivationPath(this.hdPathString)
-
-  keyStore.generateNewAddress(derivedKey, 1)
-  configManager.setWallet(keyStore.serialize())
-  if (global.METAMASK_DEBUG) {
-    console.log('restored from seed. saved to keystore')
   }
-  return keyStore
+
+  if (seedPhrase) {
+    opts.seedPhrase = seedPhrase
+  }
+
+  KeyStore.createVault(opts, (err, keyStore) => {
+    if (err) return cb(err)
+
+    this._keyStore = keyStore
+
+    keyStore.keyFromPassword(password, (err, derivedKey) => {
+      if (err) return cb(err)
+
+      this.purgeCache()
+
+      keyStore.addHdDerivationPath(this.hdPathString, derivedKey, {curve: 'secp256k1', purpose: 'sign'})
+
+      this._createFirstWallet(derivedKey)
+      this._createIdMgmt(derivedKey)
+      this.setSelectedAddressSync()
+
+      cb()
+    })
+  })
 }
 
-IdentityStore.prototype._createFirstWallet = function (entropy, derivedKey) {
+IdentityStore.prototype._createIdMgmt = function (derivedKey) {
-  const configManager = this.configManager
+  this._idmgmt = new IdManagement({
-  var secretSeed = LightwalletKeyStore.generateRandomSeed(entropy)
+    keyStore: this._keyStore,
-  var keyStore = new LightwalletKeyStore(secretSeed, derivedKey, this.hdPathString)
+    derivedKey: derivedKey,
-  keyStore.addHdDerivationPath(this.hdPathString, derivedKey, {curve: 'secp256k1', purpose: 'sign'})
+    configManager: this.configManager,
-  keyStore.setDefaultHdDerivationPath(this.hdPathString)
+  })
+}
 
+IdentityStore.prototype.purgeCache = function () {
+  this._currentState.identities = {}
+  let accounts
+  try {
+    Object.keys(this._ethStore._currentState.accounts)
+  } catch (e) {
+    accounts = []
+  }
+  accounts.forEach((address) => {
+    this._ethStore.removeAccount(address)
+  })
+}
+
+IdentityStore.prototype._createFirstWallet = function (derivedKey) {
+  const keyStore = this._keyStore
+  keyStore.setDefaultHdDerivationPath(this.hdPathString)
   keyStore.generateNewAddress(derivedKey, 1)
-  configManager.setWallet(keyStore.serialize())
+  this.configManager.setWallet(keyStore.serialize())
-  console.log('saved to keystore')
+  var addresses = keyStore.getAddresses()
-  return keyStore
+  this._ethStore.addAccount(ethUtil.addHexPrefix(addresses[0]))
 }
 
 // get addresses and normalize address hexString
 IdentityStore.prototype._getAddresses = function () {
-  return this._keyStore.getAddresses(this.hdPathString).map((address) => { return '0x' + address })
+  return this._keyStore.getAddresses(this.hdPathString).map((address) => {
+    return ethUtil.addHexPrefix(address)
+  })
 }
 
 IdentityStore.prototype._autoFaucet = function () {

development/states/account-detail.json

@@ -33,20 +33,20 @@
     "accounts": {
       "0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc": {
         "code": "0x",
-        "balance": "0x0",
+        "balance": "0x100000000000",
         "nonce": "0x0",
         "address": "0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc"
       },
       "0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b": {
         "code": "0x",
         "nonce": "0x0",
-        "balance": "0x0",
+        "balance": "0x100000000000",
         "address": "0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b"
       },
       "0xeb9e64b93097bc15f01f13eae97015c57ab64823": {
         "code": "0x",
         "nonce": "0x0",
-        "balance": "0x0",
+        "balance": "0x100000000000",
         "address": "0xeb9e64b93097bc15f01f13eae97015c57ab64823"
       },
       "0x704107d04affddd9b66ab9de3dd7b095852e9b69": {

development/states/accounts.json

@@ -29,25 +29,25 @@
     "unconfTxs": {},
     "accounts": {
       "0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc": {
-        "balance": "0x0",
+        "balance": "0x100000000000000",
         "nonce": "0x0",
         "code": "0x",
         "address": "0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc"
       },
       "0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b": {
-        "balance": "0x0",
+        "balance": "0x100000000000000",
         "nonce": "0x0",
         "code": "0x",
         "address": "0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b"
       },
       "0xeb9e64b93097bc15f01f13eae97015c57ab64823": {
-        "balance": "0x0",
+        "balance": "0x100000000000",
         "nonce": "0x0",
         "code": "0x",
         "address": "0xeb9e64b93097bc15f01f13eae97015c57ab64823"
       },
       "0x704107d04affddd9b66ab9de3dd7b095852e9b69": {
-        "balance": "0x0",
+        "balance": "0x100000000000",
         "code": "0x",
         "nonce": "0x0",
         "address": "0x704107d04affddd9b66ab9de3dd7b095852e9b69"
@@ -82,4 +82,4 @@
     "scrollToBottom": true
   },
   "identities": {}
-}
+}

test/unit/idStore-test.js

@@ -1,6 +1,8 @@
 var assert = require('assert')
 var IdentityStore = require('../../app/scripts/lib/idStore')
 var configManagerGen = require('../lib/mock-config-manager')
+const ethUtil = require('ethereumjs-util')
+const async = require('async')
 
 describe('IdentityStore', function() {
 
@@ -18,11 +20,12 @@ describe('IdentityStore', function() {
       idStore = new IdentityStore({
         configManager: configManagerGen(),
         ethStore: {
-          addAccount(acct) { accounts.push(acct) },
+          addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) },
         },
       })
 
       idStore.createNewVault(password, entropy, (err, seeds) => {
+        assert.ifError(err, 'createNewVault threw error')
         seedWords = seeds
         originalKeystore = idStore._idmgmt.keyStore
         done()
@@ -38,7 +41,7 @@ describe('IdentityStore', function() {
         idStore = new IdentityStore({
           configManager: configManagerGen(),
           ethStore: {
-            addAccount(acct) { newAccounts.push(acct) },
+            addAccount(acct) { newAccounts.push(ethUtil.addHexPrefix(acct)) },
           },
         })
       })
@@ -57,31 +60,80 @@ describe('IdentityStore', function() {
   })
 
   describe('#recoverFromSeed BIP44 compliance', function() {
-    let seedWords =  'picnic injury awful upper eagle junk alert toss flower renew silly vague'
+   const salt = 'lightwalletSalt'
-    let firstAccount = '0x5d8de92c205279c10e5669f797b853ccef4f739a'
 
     let password = 'secret!'
-    let accounts = []
+    let accounts = {}
     let idStore
 
+    var assertions = [
+      {
+        seed: 'picnic injury awful upper eagle junk alert toss flower renew silly vague',
+        account: '0x5d8de92c205279c10e5669f797b853ccef4f739a',
+      },
+      {
+        seed: 'radar blur cabbage chef fix engine embark joy scheme fiction master release',
+        account: '0xe15d894becb0354c501ae69429b05143679f39e0',
+      },
+      {
+         seed: 'phone coyote caught pattern found table wedding list tumble broccoli chief swing',
+        account: '0xb0e868f24bc7fec2bce2efc2b1c344d7569cd9d2',
+      },
+      {
+        seed: 'recycle tag bird palace blue village anxiety census cook soldier example music',
+        account: '0xab34a45920afe4af212b96ec51232aaa6a33f663',
+      },
+      {
+        seed: 'half glimpse tape cute harvest sweet bike voyage actual floor poet lazy',
+        account: '0x28e9044597b625ac4beda7250011670223de43b2',
+      },
+      {
+        seed: 'flavor tiger carpet motor angry hungry document inquiry large critic usage liar',
+        account: '0xb571be96558940c4e9292e1999461aa7499fb6cd',
+      },
+    ]
+
     before(function() {
       window.localStorage = {} // Hacking localStorage support into JSDom
 
       idStore = new IdentityStore({
         configManager: configManagerGen(),
         ethStore: {
-          addAccount(acct) { accounts.push(acct) },
+          addAccount(acct) { accounts[acct] = acct},
+          del(acct) { delete accounts[acct] },
         },
       })
     })
 
-    it('should return the expected first account', function (done) {
+    it('should enforce seed compliance with TestRPC', function (done) {
+      this.timeout(10000)
+      const tests = assertions.map((assertion) => {
+        return function (cb) {
 
-      idStore.recoverFromSeed(password, seedWords, (err) => {
+          idStore.recoverFromSeed(password, assertion.seed, (err) => {
+            assert.ifError(err)
+
+            var expected = assertion.account.toLowerCase()
+            var received = accounts[expected].toLowerCase()
+            assert.equal(received, expected)
+
+            idStore.tryPassword(password, function (err) {
+
+              assert.ok(idStore._isUnlocked(), 'should unlock the id store')
+
+              idStore.submitPassword(password, function(err, account) {
+                assert.ifError(err)
+                assert.equal(account, expected)
+                assert.equal(Object.keys(idStore._getAddresses()).length, 1, 'only one account on restore')
+                cb()
+              })
+            })
+          })
+        }
+      })
+
+      async.series(tests, function(err, results) {
         assert.ifError(err)
-
-        let newKeystore = idStore._idmgmt.keyStore
-        assert.equal(accounts[0], firstAccount)
         done()
       })
     })

ui/app/accounts/account-list-item.js

@@ -7,14 +7,14 @@ const EthBalance = require('../components/eth-balance')
 const CopyButton = require('../components/copyButton')
 const Identicon = require('../components/identicon')
 
-module.exports = NewComponent
+module.exports = AccountListItem
 
-inherits(NewComponent, Component)
+inherits(AccountListItem, Component)
-function NewComponent () {
+function AccountListItem () {
   Component.call(this)
 }
 
-NewComponent.prototype.render = function () {
+AccountListItem.prototype.render = function () {
   const identity = this.props.identity
   var isSelected = this.props.selectedAddress === identity.address
   var account = this.props.accounts[identity.address]
@@ -23,9 +23,6 @@ NewComponent.prototype.render = function () {
   return (
     h(`.accounts-list-option.flex-row.flex-space-between.pointer.hover-white${selectedClass}`, {
       key: `account-panel-${identity.address}`,
-      style: {
-        flex: '1 0 auto',
-      },
       onClick: (event) => this.props.onShowDetail(identity.address, event),
     }, [
 
@@ -73,7 +70,7 @@ NewComponent.prototype.render = function () {
   )
 }
 
-NewComponent.prototype.pendingOrNot = function () {
+AccountListItem.prototype.pendingOrNot = function () {
   const pending = this.props.pending
   if (pending.length === 0) return null
   return h('.pending-dot', pending.length)

ui/app/accounts/index.js

@@ -84,7 +84,7 @@ AccountsScreen.prototype.render = function () {
             })
           }),
 
-          h('hr.horizontal-line', {key: 'horizontal-line1'}),
+          h('hr.horizontal-line'),
           h('div.footer.hover-white.pointer', {
             key: 'reveal-account-bar',
             onClick: () => {
@@ -92,7 +92,6 @@ AccountsScreen.prototype.render = function () {
             },
             style: {
               display: 'flex',
-              flex: '1 0 auto',
               height: '40px',
               paddint: '10px',
               justifyContent: 'center',
@@ -101,6 +100,7 @@ AccountsScreen.prototype.render = function () {
           }, [
             h('i.fa.fa-plus.fa-lg', {key: ''}),
           ]),
+          h('hr.horizontal-line'),
         ]),
 
       unconfTxList.length ? (

ui/app/loading.js

@@ -30,6 +30,7 @@ LoadingIndicator.prototype.render = function () {
 
       isLoading ? h('div', {
         style: {
+          zIndex: 10,
           position: 'absolute',
           display: 'flex',
           justifyContent: 'center',