From 1547af191bfecbb7c37d8ace3f6a786fa4eb1edc Mon Sep 17 00:00:00 2001 From: Jannis Redmann Date: Thu, 2 Feb 2017 16:01:37 +0100 Subject: [PATCH] more permissive verification process (#4317) * style fixes :sparkles: * verification: find last request * verification: don't request again if same input * didRequestWithSameValues -> shallRequestAgain * bugfixes :bug:, update SMS verification ABI * verification: hasRequested -> accountHasRequested * verification: hasIsVerified -> accountIsVerified * verification: shallRequestAgain -> shallSkipRequest * verification: show if unable to send req * email verification: check if email already used * address style grumbles :art: --- js/src/contracts/abi/sms-verification.json | 2 +- js/src/contracts/verification.js | 12 +-- .../Verification/GatherData/gatherData.css | 4 + .../Verification/GatherData/gatherData.js | 77 +++++++++++----- js/src/modals/Verification/email-store.js | 52 ++++++++++- js/src/modals/Verification/sms-store.js | 14 ++- js/src/modals/Verification/store.js | 92 +++++++++++-------- js/src/modals/Verification/verification.js | 41 +++++++-- 8 files changed, 214 insertions(+), 80 deletions(-) diff --git a/js/src/contracts/abi/sms-verification.json b/js/src/contracts/abi/sms-verification.json index d6852b182..3eb5492a4 100644 --- a/js/src/contracts/abi/sms-verification.json +++ b/js/src/contracts/abi/sms-verification.json @@ -1 +1 @@ -[{"constant":false,"inputs":[{"name":"_new","type":"address"}],"name":"setOwner","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_who","type":"address"}],"name":"certify","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[],"name":"request","outputs":[],"payable":true,"type":"function"},{"constant":false,"inputs":[{"name":"_who","type":"address"},{"name":"_puzzle","type":"bytes32"}],"name":"puzzle","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"},{"name":"_field","type":"string"}],"name":"getAddress","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_new","type":"uint256"}],"name":"setFee","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_who","type":"address"}],"name":"revoke","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_code","type":"bytes32"}],"name":"confirm","outputs":[{"name":"","type":"bool"}],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"owner","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"},{"constant":false,"inputs":[],"name":"drain","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"delegate","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"},{"name":"_field","type":"string"}],"name":"getUint","outputs":[{"name":"","type":"uint256"}],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_new","type":"address"}],"name":"setDelegate","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"}],"name":"certified","outputs":[{"name":"","type":"bool"}],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"fee","outputs":[{"name":"","type":"uint256"}],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"},{"name":"_field","type":"string"}],"name":"get","outputs":[{"name":"","type":"bytes32"}],"payable":false,"type":"function"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"}],"name":"Requested","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"},{"indexed":false,"name":"puzzle","type":"bytes32"}],"name":"Puzzled","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"}],"name":"Confirmed","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"}],"name":"Revoked","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"old","type":"address"},{"indexed":true,"name":"current","type":"address"}],"name":"NewOwner","type":"event"}] +[{"constant":false,"inputs":[{"name":"_new","type":"address"}],"name":"setOwner","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_who","type":"address"}],"name":"certify","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[],"name":"request","outputs":[],"payable":true,"type":"function"},{"constant":false,"inputs":[{"name":"_who","type":"address"},{"name":"_puzzle","type":"bytes32"}],"name":"puzzle","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"},{"name":"_field","type":"string"}],"name":"getAddress","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_new","type":"uint256"}],"name":"setFee","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_who","type":"address"}],"name":"revoke","outputs":[],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_code","type":"bytes32"}],"name":"confirm","outputs":[{"name":"","type":"bool"}],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"owner","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"},{"constant":false,"inputs":[],"name":"drain","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"delegate","outputs":[{"name":"","type":"address"}],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"},{"name":"_field","type":"string"}],"name":"getUint","outputs":[{"name":"","type":"uint256"}],"payable":false,"type":"function"},{"constant":false,"inputs":[{"name":"_new","type":"address"}],"name":"setDelegate","outputs":[],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"}],"name":"certified","outputs":[{"name":"","type":"bool"}],"payable":false,"type":"function"},{"constant":true,"inputs":[],"name":"fee","outputs":[{"name":"","type":"uint256"}],"payable":false,"type":"function"},{"constant":true,"inputs":[{"name":"_who","type":"address"},{"name":"_field","type":"string"}],"name":"get","outputs":[{"name":"","type":"bytes32"}],"payable":false,"type":"function"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"}],"name":"Requested","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"},{"indexed":false,"name":"puzzle","type":"bytes32"}],"name":"Puzzled","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"}],"name":"Confirmed","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"who","type":"address"}],"name":"Revoked","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"old","type":"address"},{"indexed":true,"name":"current","type":"address"}],"name":"NewOwner","type":"event"}] \ No newline at end of file diff --git a/js/src/contracts/verification.js b/js/src/contracts/verification.js index 2d69fa8d6..8101565db 100644 --- a/js/src/contracts/verification.js +++ b/js/src/contracts/verification.js @@ -20,23 +20,23 @@ export const checkIfVerified = (contract, account) => { return contract.instance.certified.call({}, [account]); }; -export const checkIfRequested = (contract, account) => { +export const findLastRequested = (contract, account) => { let subId = null; let resolved = false; return new Promise((resolve, reject) => { contract .subscribe('Requested', { - fromBlock: 0, toBlock: 'pending' + fromBlock: 0, + toBlock: 'pending', + limit: 1, + topics: [account] }, (err, logs) => { if (err) { return reject(err); } - const e = logs.find((l) => { - return l.type === 'mined' && l.params.who && l.params.who.value === account; - }); - resolve(e ? e.transactionHash : false); + resolve(logs[0] || null); resolved = true; if (subId) { diff --git a/js/src/modals/Verification/GatherData/gatherData.css b/js/src/modals/Verification/GatherData/gatherData.css index e0c0f8b57..0f9ba3617 100644 --- a/js/src/modals/Verification/GatherData/gatherData.css +++ b/js/src/modals/Verification/GatherData/gatherData.css @@ -30,6 +30,10 @@ margin-left: .5em; } +.field { + margin-bottom: .5em; +} + .terms { line-height: 1.3; opacity: .7; diff --git a/js/src/modals/Verification/GatherData/gatherData.js b/js/src/modals/Verification/GatherData/gatherData.js index e6f21136c..ecd096be6 100644 --- a/js/src/modals/Verification/GatherData/gatherData.js +++ b/js/src/modals/Verification/GatherData/gatherData.js @@ -31,19 +31,22 @@ import emailTermsOfService from '~/3rdparty/email-verification/terms-of-service' import { howSMSVerificationWorks, howEmailVerificationWorks } from '../how-it-works'; import styles from './gatherData.css'; +const boolOfError = PropTypes.oneOfType([ PropTypes.bool, PropTypes.instanceOf(Error) ]); + export default class GatherData extends Component { static propTypes = { fee: React.PropTypes.instanceOf(BigNumber), fields: PropTypes.array.isRequired, - hasRequested: nullableProptype(PropTypes.bool.isRequired), + accountHasRequested: nullableProptype(PropTypes.bool.isRequired), isServerRunning: nullableProptype(PropTypes.bool.isRequired), - isVerified: nullableProptype(PropTypes.bool.isRequired), + isAbleToRequest: nullableProptype(boolOfError.isRequired), + accountIsVerified: nullableProptype(PropTypes.bool.isRequired), method: PropTypes.string.isRequired, setConsentGiven: PropTypes.func.isRequired } render () { - const { method, isVerified } = this.props; + const { method, accountIsVerified } = this.props; const termsOfService = method === 'email' ? emailTermsOfService : smsTermsOfService; const howItWorks = method === 'email' ? howEmailVerificationWorks : howSMSVerificationWorks; @@ -55,6 +58,7 @@ export default class GatherData extends Component { { this.renderCertified() } { this.renderRequested() } { this.renderFields() } + { this.renderIfAbleToRequest() } } - disabled={ isVerified } + disabled={ accountIsVerified } onCheck={ this.consentOnChange } />
{ termsOfService }
@@ -145,27 +149,27 @@ export default class GatherData extends Component { } renderCertified () { - const { isVerified } = this.props; + const { accountIsVerified } = this.props; - if (isVerified) { + if (accountIsVerified) { return (

); - } else if (isVerified === false) { + } else if (accountIsVerified === false) { return (

@@ -175,7 +179,7 @@ export default class GatherData extends Component { return (

@@ -183,33 +187,33 @@ export default class GatherData extends Component { } renderRequested () { - const { isVerified, hasRequested } = this.props; + const { accountIsVerified, accountHasRequested } = this.props; // If the account is verified, don't show that it has requested verification. - if (isVerified) { + if (accountIsVerified) { return null; } - if (hasRequested) { + if (accountHasRequested) { return (

); - } else if (hasRequested === false) { + } else if (accountHasRequested === false) { return (

@@ -218,7 +222,7 @@ export default class GatherData extends Component { return (

@@ -226,7 +230,7 @@ export default class GatherData extends Component { } renderFields () { - const { isVerified, fields } = this.props; + const { accountIsVerified, fields } = this.props; const rendered = fields.map((field) => { const onChange = (_, v) => { @@ -236,11 +240,12 @@ export default class GatherData extends Component { return ( @@ -250,6 +255,36 @@ export default class GatherData extends Component { return (
{rendered}
); } + renderIfAbleToRequest () { + const { accountIsVerified, isAbleToRequest } = this.props; + + // If the account is verified, don't show a warning. + // If the client is able to send the request, don't show a warning + if (accountIsVerified || isAbleToRequest === true) { + return null; + } + + if (isAbleToRequest === null) { + return ( +

+ +

+ ); + } else if (isAbleToRequest) { + return ( +
+ +

+ { isAbleToRequest.message } +

+
+ ); + } + } + consentOnChange = (_, consentGiven) => { this.props.setConsentGiven(consentGiven); } diff --git a/js/src/modals/Verification/email-store.js b/js/src/modals/Verification/email-store.js index b69764926..d18d5fac9 100644 --- a/js/src/modals/Verification/email-store.js +++ b/js/src/modals/Verification/email-store.js @@ -16,6 +16,7 @@ import { observable, computed, action } from 'mobx'; import { sha3 } from '~/api/util/sha3'; +import { bytesToHex } from '~/api/util/format'; import EmailVerificationABI from '~/contracts/abi/email-verification.json'; import VerificationStore, { @@ -23,6 +24,8 @@ import VerificationStore, { } from './store'; import { isServerRunning, hasReceivedCode, postToServer } from '~/3rdparty/email-verification'; +const ZERO20 = '0x0000000000000000000000000000000000000000'; + // name in the `BadgeReg.sol` contract const EMAIL_VERIFICATION = 'emailverification'; @@ -44,9 +47,9 @@ export default class EmailVerificationStore extends VerificationStore { switch (this.step) { case LOADING: - return this.contract && this.fee && this.isVerified !== null && this.hasRequested !== null; + return this.contract && this.fee && this.accountIsVerified !== null && this.accountHasRequested !== null; case QUERY_DATA: - return this.isEmailValid && this.consentGiven; + return this.isEmailValid && this.consentGiven && this.isAbleToRequest === true; case QUERY_CODE: return this.requestTx && this.isCodeValid === true; case POSTED_CONFIRMATION: @@ -68,8 +71,53 @@ export default class EmailVerificationStore extends VerificationStore { return hasReceivedCode(this.email, this.account, this.isTestnet); } + // If the email has already been used for verification of another account, + // we prevent the user from wasting ETH to request another verification. + @action setIfAbleToRequest = () => { + const { isEmailValid } = this; + + if (!isEmailValid) { + this.isAbleToRequest = true; + return; + } + + const { contract, email } = this; + const emailHash = sha3.text(email); + + this.isAbleToRequest = null; + contract + .instance.reverse + .call({}, [ emailHash ]) + .then((address) => { + if (address === ZERO20) { + this.isAbleToRequest = true; + } else { + this.isAbleToRequest = new Error('Another account has been verified using this e-mail.'); + } + }) + .catch((err) => { + this.error = 'Failed to check if able to send request: ' + err.message; + }); + } + + // Determine the values relevant for checking if the last request contains + // the same data as the current one. requestValues = () => [ sha3.text(this.email) ] + shallSkipRequest = (currentValues) => { + const { accountHasRequested } = this; + const lastRequest = this.lastRequestValues; + + if (!accountHasRequested) { + return Promise.resolve(false); + } + // If the last email verification `request` for the selected address contains + // the same email as the current one, don't send another request to save ETH. + const skip = currentValues[0] === bytesToHex(lastRequest.emailHash.value); + + return Promise.resolve(skip); + } + @action setEmail = (email) => { this.email = email; } diff --git a/js/src/modals/Verification/sms-store.js b/js/src/modals/Verification/sms-store.js index ee57a29ec..25c07403c 100644 --- a/js/src/modals/Verification/sms-store.js +++ b/js/src/modals/Verification/sms-store.js @@ -43,7 +43,7 @@ export default class SMSVerificationStore extends VerificationStore { switch (this.step) { case LOADING: - return this.contract && this.fee && this.isVerified !== null && this.hasRequested !== null; + return this.contract && this.fee && this.accountIsVerified !== null && this.accountHasRequested !== null; case QUERY_DATA: return this.isNumberValid && this.consentGiven; case QUERY_CODE: @@ -67,6 +67,18 @@ export default class SMSVerificationStore extends VerificationStore { return hasReceivedCode(this.number, this.account, this.isTestnet); } + // SMS verification events don't contain the phone number, so we will have to + // send a new request every single time. See below. + @action setIfAbleToRequest = () => { + this.isAbleToRequest = true; + } + + // SMS verification `request` & `confirm` transactions and events don't contain the + // phone number, so we will have to send a new request every single time. This may + // cost the user more money, but given that it fails otherwise, it seems like a + // reasonable tradeoff. + shallSkipRequest = () => Promise.resolve(false) + @action setNumber = (number) => { this.number = number; } diff --git a/js/src/modals/Verification/store.js b/js/src/modals/Verification/store.js index a0eaba2ee..b652131d9 100644 --- a/js/src/modals/Verification/store.js +++ b/js/src/modals/Verification/store.js @@ -19,7 +19,7 @@ import { sha3 } from '~/api/util/sha3'; import Contract from '~/api/contract'; import Contracts from '~/contracts'; -import { checkIfVerified, checkIfRequested, awaitPuzzle } from '~/contracts/verification'; +import { checkIfVerified, findLastRequested, awaitPuzzle } from '~/contracts/verification'; import { checkIfTxFailed, waitForConfirmations } from '~/util/tx'; export const LOADING = 'fetching-contract'; @@ -38,8 +38,10 @@ export default class VerificationStore { @observable contract = null; @observable fee = null; - @observable isVerified = null; - @observable hasRequested = null; + @observable accountIsVerified = null; + @observable accountHasRequested = null; + @observable isAbleToRequest = null; + @observable lastRequestValues = null; @observable isServerRunning = null; @observable consentGiven = false; @observable requestTx = null; @@ -68,6 +70,14 @@ export default class VerificationStore { console.error('verification: ' + this.error); } }); + + autorun(() => { + if (this.step !== QUERY_DATA) { + return; + } + + this.setIfAbleToRequest(); + }); } @action load = () => { @@ -91,19 +101,20 @@ export default class VerificationStore { this.error = 'Failed to fetch the fee: ' + err.message; }); - const isVerified = checkIfVerified(contract, account) - .then((isVerified) => { - this.isVerified = isVerified; + const accountIsVerified = checkIfVerified(contract, account) + .then((accountIsVerified) => { + this.accountIsVerified = accountIsVerified; }) .catch((err) => { this.error = 'Failed to check if verified: ' + err.message; }); - const hasRequested = checkIfRequested(contract, account) - .then((txHash) => { - this.hasRequested = !!txHash; - if (txHash) { - this.requestTx = txHash; + const accountHasRequested = findLastRequested(contract, account) + .then((log) => { + this.accountHasRequested = !!log; + if (log) { + this.lastRequestValues = log.params; + this.requestTx = log.transactionHash; } }) .catch((err) => { @@ -111,7 +122,7 @@ export default class VerificationStore { }); Promise - .all([ isServerRunning, fee, isVerified, hasRequested ]) + .all([ isServerRunning, fee, accountIsVerified, accountHasRequested ]) .then(() => { this.step = QUERY_DATA; }); @@ -150,40 +161,41 @@ export default class VerificationStore { requestValues = () => [] @action sendRequest = () => { - const { api, account, contract, fee, hasRequested } = this; + const { api, account, contract, fee } = this; const request = contract.functions.find((fn) => fn.name === 'request'); const options = { from: account, value: fee.toString() }; const values = this.requestValues(); - let chain = Promise.resolve(); + this.shallSkipRequest(values) + .then((skipRequest) => { + if (skipRequest) { + return; + } - if (!hasRequested) { - this.step = POSTING_REQUEST; - chain = request.estimateGas(options, values) - .then((gas) => { - options.gas = gas.mul(1.2).toFixed(0); - return request.postTransaction(options, values); - }) - .then((handle) => { - // TODO: The "request rejected" error doesn't have any property to - // distinguish it from other errors, so we can't give a meaningful error here. - return api.pollMethod('parity_checkRequest', handle); - }) - .then((txHash) => { - this.requestTx = txHash; - return checkIfTxFailed(api, txHash, options.gas) - .then((hasFailed) => { - if (hasFailed) { - throw new Error('Transaction failed, all gas used up.'); - } - this.step = POSTED_REQUEST; - return waitForConfirmations(api, txHash, 1); - }); - }); - } - - chain + this.step = POSTING_REQUEST; + return request.estimateGas(options, values) + .then((gas) => { + options.gas = gas.mul(1.2).toFixed(0); + return request.postTransaction(options, values); + }) + .then((handle) => { + // The "request rejected" error doesn't have any property to distinguish + // it from other errors, so we can't give a meaningful error here. + return api.pollMethod('parity_checkRequest', handle); + }) + .then((txHash) => { + this.requestTx = txHash; + return checkIfTxFailed(api, txHash, options.gas) + .then((hasFailed) => { + if (hasFailed) { + throw new Error('Transaction failed, all gas used up.'); + } + this.step = POSTED_REQUEST; + return waitForConfirmations(api, txHash, 1); + }); + }); + }) .then(() => this.checkIfReceivedCode()) .then((hasReceived) => { if (hasReceived) { diff --git a/js/src/modals/Verification/verification.js b/js/src/modals/Verification/verification.js index 4e179eb43..37166b4af 100644 --- a/js/src/modals/Verification/verification.js +++ b/js/src/modals/Verification/verification.js @@ -15,6 +15,7 @@ // along with Parity. If not, see . import React, { Component, PropTypes } from 'react'; +import { FormattedMessage } from 'react-intl'; import { connect } from 'react-redux'; import { observer } from 'mobx-react'; import { observable } from 'mobx'; @@ -206,7 +207,7 @@ class Verification extends Component { const { step, - isServerRunning, fee, isVerified, hasRequested, + isServerRunning, isAbleToRequest, fee, accountIsVerified, accountHasRequested, requestTx, isCodeValid, confirmationTx, setCode } = this.store; @@ -223,17 +224,37 @@ class Verification extends Component { if (method === 'sms') { fields.push({ key: 'number', - label: 'phone number in international format', - hint: 'the SMS will be sent to this number', + label: ( + + ), + hint: ( + + ), error: this.store.isNumberValid ? null : 'invalid number', onChange: this.store.setNumber }); } else if (method === 'email') { fields.push({ key: 'email', - label: 'email address', - hint: 'the code will be sent to this address', - error: this.store.isEmailValid ? null : 'invalid email', + label: ( + + ), + hint: ( + + ), + error: this.store.isEmailValid ? null : 'invalid e-mail', onChange: this.store.setEmail }); } @@ -241,10 +262,12 @@ class Verification extends Component { return ( );