From ca196d683ef9c18bead8ccc5fa399ef6de5e5e7f Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Mon, 30 Jan 2017 15:08:02 +0100 Subject: [PATCH] Fix postsign (#4347) * Fix whitespace. * Fix post sign. * Fix message. * Fix tests. * Rest of the problems. * All hail the linter and its omniscience. * ...and its divine omniscience. * Grumbles and wording. --- js/src/api/rpc/parity/parity.js | 5 ++ .../RequestPending/requestPending.js | 2 +- .../components/SignRequest/signRequest.css | 14 ++++- .../components/SignRequest/signRequest.js | 56 +++++++++++++------ .../transactionPendingFormConfirm.js | 5 +- .../transactionPendingFormReject.js | 4 +- .../transactionPendingForm.js | 2 +- rpc/src/v1/helpers/dispatch.rs | 9 +-- rpc/src/v1/helpers/requests.rs | 4 +- rpc/src/v1/impls/signing.rs | 9 ++- rpc/src/v1/impls/signing_unsafe.rs | 6 +- rpc/src/v1/tests/mocked/signer.rs | 6 +- rpc/src/v1/traits/parity_signing.rs | 4 +- rpc/src/v1/types/confirmations.rs | 21 +++---- 14 files changed, 94 insertions(+), 53 deletions(-) diff --git a/js/src/api/rpc/parity/parity.js b/js/src/api/rpc/parity/parity.js index 02fd3d53e..92fc2721d 100644 --- a/js/src/api/rpc/parity/parity.js +++ b/js/src/api/rpc/parity/parity.js @@ -297,6 +297,11 @@ export default class Parity { .execute('parity_postTransaction', inOptions(options)); } + postSign (from, message) { + return this._transport + .execute('parity_postSign', from, message); + } + registryAddress () { return this._transport .execute('parity_registryAddress') diff --git a/js/src/views/Signer/components/RequestPending/requestPending.js b/js/src/views/Signer/components/RequestPending/requestPending.js index 4194fc1dd..f860fcd00 100644 --- a/js/src/views/Signer/components/RequestPending/requestPending.js +++ b/js/src/views/Signer/components/RequestPending/requestPending.js @@ -61,7 +61,7 @@ export default class RequestPending extends Component { address={ sign.address } className={ className } focus={ focus } - hash={ sign.hash } + data={ sign.data } id={ id } isFinished={ false } isSending={ isSending } diff --git a/js/src/views/Signer/components/SignRequest/signRequest.css b/js/src/views/Signer/components/SignRequest/signRequest.css index e8a64e91f..b26932e19 100644 --- a/js/src/views/Signer/components/SignRequest/signRequest.css +++ b/js/src/views/Signer/components/SignRequest/signRequest.css @@ -27,8 +27,20 @@ min-height: $pendingHeight; } +.signData { + border: 0.25em solid red; + padding: 0.5em; + margin-left: -2em; + overflow: auto; + max-height: 6em; +} + +.signData > p { + color: white; +} + .signDetails { - flex: 1; + flex: 10; } .address, .info { diff --git a/js/src/views/Signer/components/SignRequest/signRequest.js b/js/src/views/Signer/components/SignRequest/signRequest.js index 28e9fbf7d..4a0745382 100644 --- a/js/src/views/Signer/components/SignRequest/signRequest.js +++ b/js/src/views/Signer/components/SignRequest/signRequest.js @@ -19,16 +19,30 @@ import { observer } from 'mobx-react'; import Account from '../Account'; import TransactionPendingForm from '../TransactionPendingForm'; -import TxHashLink from '../TxHashLink'; import styles from './signRequest.css'; +function isAscii (data) { + for (var i = 2; i < data.length; i += 2) { + let n = parseInt(data.substr(i, 2), 16); + + if (n < 32 || n >= 128) { + return false; + } + } + return true; +} + @observer export default class SignRequest extends Component { + static contextTypes = { + api: PropTypes.object + }; + static propTypes = { id: PropTypes.object.isRequired, address: PropTypes.string.isRequired, - hash: PropTypes.string.isRequired, + data: PropTypes.string.isRequired, isFinished: PropTypes.bool.isRequired, isTest: PropTypes.bool.isRequired, store: PropTypes.object.isRequired, @@ -62,8 +76,23 @@ export default class SignRequest extends Component { ); } + renderAsciiDetails (ascii) { + return ( +
+

{ascii}

+
+ ); + } + + renderBinaryDetails (data) { + return (
+

(Unknown binary data)

+
); + } + renderDetails () { - const { address, hash, isTest, store } = this.props; + const { api } = this.context; + const { address, isTest, store, data } = this.props; const balance = store.balances[address]; if (!balance) { @@ -79,9 +108,14 @@ export default class SignRequest extends Component { isTest={ isTest } /> -
-

Dapp is requesting to sign arbitrary transaction using this account.

-

Confirm the transaction only if you trust the app.

+
+

A request to sign data using your account:

+ { + isAscii(data) + ? this.renderAsciiDetails(api.util.hexToAscii(data)) + : this.renderBinaryDetails(data) + } +

WARNING: This consequences of doing this may be grave. Confirm the request only if you are sure.

); @@ -92,19 +126,9 @@ export default class SignRequest extends Component { if (isFinished) { if (status === 'confirmed') { - const { hash, isTest } = this.props; - return (
Confirmed -
- Transaction hash: - -
); } diff --git a/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js b/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js index 042d4b400..224b1c2b4 100644 --- a/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js +++ b/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormConfirm/transactionPendingFormConfirm.js @@ -150,7 +150,7 @@ class TransactionPendingFormConfirm extends Component { label={ isSending ? 'Confirming...' - : 'Confirm Transaction' + : 'Confirm Request' } onTouchTap={ this.onConfirm } primary @@ -256,7 +256,8 @@ function mapStateToProps (_, initProps) { return (state) => { const { accounts } = state.personal; - const account = accounts[address] || {}; + let gotAddress = Object.keys(accounts).find(a => a.toLowerCase() === address); + const account = gotAddress ? accounts[gotAddress] : {}; return { account }; }; diff --git a/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormReject/transactionPendingFormReject.js b/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormReject/transactionPendingFormReject.js index 1f4c0df5f..01767c24a 100644 --- a/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormReject/transactionPendingFormReject.js +++ b/js/src/views/Signer/components/TransactionPendingForm/TransactionPendingFormReject/transactionPendingFormReject.js @@ -32,14 +32,14 @@ export default class TransactionPendingFormReject extends Component { return (
- Are you sure you want to reject transaction?
+ Are you sure you want to reject request?
This cannot be undone
); diff --git a/js/src/views/Signer/components/TransactionPendingForm/transactionPendingForm.js b/js/src/views/Signer/components/TransactionPendingForm/transactionPendingForm.js index aa32e0eb9..9b114759e 100644 --- a/js/src/views/Signer/components/TransactionPendingForm/transactionPendingForm.js +++ b/js/src/views/Signer/components/TransactionPendingForm/transactionPendingForm.js @@ -75,7 +75,7 @@ export default class TransactionPendingForm extends Component { let html; if (!isRejectOpen) { - html = reject transaction; + html = reject request; } else { html = { "I've changed my mind" }; } diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 615ecd507..c0d8d04eb 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -19,6 +19,7 @@ use std::ops::Deref; use rlp; use util::{Address, H256, U256, Uint, Bytes}; use util::bytes::ToPretty; +use util::sha3::Hashable; use ethkey::Signature; use ethcore::miner::MinerService; @@ -108,8 +109,8 @@ pub fn execute(client: &C, miner: &M, accounts: &AccountProvider, payload: .map(ConfirmationResponse::SignTransaction) ) }, - ConfirmationPayload::Signature(address, hash) => { - signature(accounts, address, hash, pass) + ConfirmationPayload::Signature(address, data) => { + signature(accounts, address, data.sha3(), pass) .map(|result| result .map(RpcH520::from) .map(ConfirmationResponse::Signature) @@ -243,8 +244,8 @@ pub fn from_rpc(payload: RpcConfirmationPayload, client: &C, miner: &M) -> RpcConfirmationPayload::Decrypt(RpcDecryptRequest { address, msg }) => { ConfirmationPayload::Decrypt(address.into(), msg.into()) }, - RpcConfirmationPayload::Signature(RpcSignRequest { address, hash }) => { - ConfirmationPayload::Signature(address.into(), hash.into()) + RpcConfirmationPayload::Signature(RpcSignRequest { address, data }) => { + ConfirmationPayload::Signature(address.into(), data.into()) }, } } diff --git a/rpc/src/v1/helpers/requests.rs b/rpc/src/v1/helpers/requests.rs index 04ba8ce77..2bf51f744 100644 --- a/rpc/src/v1/helpers/requests.rs +++ b/rpc/src/v1/helpers/requests.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -use util::{Address, U256, Bytes, H256}; +use util::{Address, U256, Bytes}; /// Transaction request coming from RPC #[derive(Debug, Clone, Default, Eq, PartialEq, Hash)] @@ -109,7 +109,7 @@ pub enum ConfirmationPayload { /// Sign Transaction SignTransaction(FilledTransactionRequest), /// Sign request - Signature(Address, H256), + Signature(Address, Bytes), /// Decrypt request Decrypt(Address, Bytes), } diff --git a/rpc/src/v1/impls/signing.rs b/rpc/src/v1/impls/signing.rs index e8cb73c96..5ed215281 100644 --- a/rpc/src/v1/impls/signing.rs +++ b/rpc/src/v1/impls/signing.rs @@ -18,7 +18,7 @@ use std::sync::{Arc, Weak}; use transient_hashmap::TransientHashMap; -use util::{U256, Mutex, Hashable}; +use util::{U256, Mutex}; use ethcore::account_provider::AccountProvider; use ethcore::miner::MinerService; @@ -122,9 +122,9 @@ impl ParitySigning for SigningQueueClient where C: MiningBlockChainClient, M: MinerService, { - fn post_sign(&self, address: RpcH160, hash: RpcH256) -> Result, Error> { + fn post_sign(&self, address: RpcH160, data: RpcBytes) -> Result, Error> { self.active()?; - self.dispatch(RpcConfirmationPayload::Signature((address, hash).into())) + self.dispatch(RpcConfirmationPayload::Signature((address, data).into())) .map(|result| match result { DispatchResult::Value(v) => RpcEither::Or(v), DispatchResult::Promise(promise) => { @@ -187,8 +187,7 @@ impl EthSigning for SigningQueueClient where M: MinerService, { fn sign(&self, address: RpcH160, data: RpcBytes) -> BoxFuture { - let hash = data.0.sha3().into(); - let res = self.active().and_then(|_| self.dispatch(RpcConfirmationPayload::Signature((address, hash).into()))); + let res = self.active().and_then(|_| self.dispatch(RpcConfirmationPayload::Signature((address, data).into()))); let (ready, p) = futures::oneshot(); self.handle_dispatch(res, |response| { diff --git a/rpc/src/v1/impls/signing_unsafe.rs b/rpc/src/v1/impls/signing_unsafe.rs index 58bbf0811..1dbffa7aa 100644 --- a/rpc/src/v1/impls/signing_unsafe.rs +++ b/rpc/src/v1/impls/signing_unsafe.rs @@ -17,7 +17,6 @@ //! Unsafe Signing RPC implementation. use std::sync::{Arc, Weak}; -use util::Hashable; use ethcore::account_provider::AccountProvider; use ethcore::miner::MinerService; @@ -86,8 +85,7 @@ impl EthSigning for SigningUnsafeClient where M: MinerService, { fn sign(&self, address: RpcH160, data: RpcBytes) -> BoxFuture { - let hash = data.0.sha3().into(); - let result = match self.handle(RpcConfirmationPayload::Signature((address, hash).into())) { + let result = match self.handle(RpcConfirmationPayload::Signature((address, data).into())) { Ok(RpcConfirmationResponse::Signature(signature)) => Ok(signature), Err(e) => Err(e), e => Err(errors::internal("Unexpected result", e)), @@ -131,7 +129,7 @@ impl ParitySigning for SigningUnsafeClient where futures::done(result).boxed() } - fn post_sign(&self, _: RpcH160, _: RpcH256) -> Result, Error> { + fn post_sign(&self, _: RpcH160, _: RpcBytes) -> Result, Error> { // We don't support this in non-signer mode. Err(errors::signer_disabled()) } diff --git a/rpc/src/v1/tests/mocked/signer.rs b/rpc/src/v1/tests/mocked/signer.rs index 4e409faaf..650f33f55 100644 --- a/rpc/src/v1/tests/mocked/signer.rs +++ b/rpc/src/v1/tests/mocked/signer.rs @@ -85,14 +85,14 @@ fn should_return_list_of_items_to_confirm() { nonce: None, min_block: None, })).unwrap(); - tester.signer.add_request(ConfirmationPayload::Signature(1.into(), 5.into())).unwrap(); + tester.signer.add_request(ConfirmationPayload::Signature(1.into(), vec![5].into())).unwrap(); // when let request = r#"{"jsonrpc":"2.0","method":"signer_requestsToConfirm","params":[],"id":1}"#; let response = concat!( r#"{"jsonrpc":"2.0","result":["#, r#"{"id":"0x1","payload":{"sendTransaction":{"data":"0x","from":"0x0000000000000000000000000000000000000001","gas":"0x989680","gasPrice":"0x2710","minBlock":null,"nonce":null,"to":"0xd46e8dd67c5d32be8058bb8eb970870f07244567","value":"0x1"}}},"#, - r#"{"id":"0x2","payload":{"sign":{"address":"0x0000000000000000000000000000000000000001","hash":"0x0000000000000000000000000000000000000000000000000000000000000005"}}}"#, + r#"{"id":"0x2","payload":{"sign":{"address":"0x0000000000000000000000000000000000000001","data":"0x05"}}}"#, r#"],"id":1}"# ); @@ -156,7 +156,7 @@ fn should_not_remove_transaction_if_password_is_invalid() { fn should_not_remove_sign_if_password_is_invalid() { // given let tester = signer_tester(); - tester.signer.add_request(ConfirmationPayload::Signature(0.into(), 5.into())).unwrap(); + tester.signer.add_request(ConfirmationPayload::Signature(0.into(), vec![5].into())).unwrap(); assert_eq!(tester.signer.requests().len(), 1); // when diff --git a/rpc/src/v1/traits/parity_signing.rs b/rpc/src/v1/traits/parity_signing.rs index d46fd1969..3ce01e345 100644 --- a/rpc/src/v1/traits/parity_signing.rs +++ b/rpc/src/v1/traits/parity_signing.rs @@ -18,7 +18,7 @@ use jsonrpc_core::Error; use futures::BoxFuture; -use v1::types::{U256, H160, H256, Bytes, ConfirmationResponse, TransactionRequest, Either}; +use v1::types::{U256, H160, Bytes, ConfirmationResponse, TransactionRequest, Either}; build_rpc_trait! { /// Signing methods implementation. @@ -26,7 +26,7 @@ build_rpc_trait! { /// Posts sign request asynchronously. /// Will return a confirmation ID for later use with check_transaction. #[rpc(name = "parity_postSign")] - fn post_sign(&self, H160, H256) -> Result, Error>; + fn post_sign(&self, H160, Bytes) -> Result, Error>; /// Posts transaction asynchronously. /// Will return a transaction ID for later use with check_transaction. diff --git a/rpc/src/v1/types/confirmations.rs b/rpc/src/v1/types/confirmations.rs index ae17fdc5f..c9b7b2006 100644 --- a/rpc/src/v1/types/confirmations.rs +++ b/rpc/src/v1/types/confirmations.rs @@ -19,6 +19,7 @@ use std::fmt; use serde::{Serialize, Serializer}; use util::log::Colour; +use util::bytes::ToPretty; use v1::types::{U256, TransactionRequest, RichRawTransaction, H160, H256, H520, Bytes, BlockNumber}; use v1::helpers; @@ -64,14 +65,14 @@ pub struct SignRequest { /// Address pub address: H160, /// Hash to sign - pub hash: H256, + pub data: Bytes, } -impl From<(H160, H256)> for SignRequest { - fn from(tuple: (H160, H256)) -> Self { +impl From<(H160, Bytes)> for SignRequest { + fn from(tuple: (H160, Bytes)) -> Self { SignRequest { address: tuple.0, - hash: tuple.1, + data: tuple.1, } } } @@ -80,8 +81,8 @@ impl fmt::Display for SignRequest { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, - "sign 0x{:?} with {}", - self.hash, + "sign 0x{} with {}", + self.data.0.pretty(), Colour::White.bold().paint(format!("0x{:?}", self.address)), ) } @@ -172,9 +173,9 @@ impl From for ConfirmationPayload { match c { helpers::ConfirmationPayload::SendTransaction(t) => ConfirmationPayload::SendTransaction(t.into()), helpers::ConfirmationPayload::SignTransaction(t) => ConfirmationPayload::SignTransaction(t.into()), - helpers::ConfirmationPayload::Signature(address, hash) => ConfirmationPayload::Signature(SignRequest { + helpers::ConfirmationPayload::Signature(address, data) => ConfirmationPayload::Signature(SignRequest { address: address.into(), - hash: hash.into(), + data: data.into(), }), helpers::ConfirmationPayload::Decrypt(address, msg) => ConfirmationPayload::Decrypt(DecryptRequest { address: address.into(), @@ -248,12 +249,12 @@ mod tests { // given let request = helpers::ConfirmationRequest { id: 15.into(), - payload: helpers::ConfirmationPayload::Signature(1.into(), 5.into()), + payload: helpers::ConfirmationPayload::Signature(1.into(), vec![5].into()), }; // when let res = serde_json::to_string(&ConfirmationRequest::from(request)); - let expected = r#"{"id":"0xf","payload":{"sign":{"address":"0x0000000000000000000000000000000000000001","hash":"0x0000000000000000000000000000000000000000000000000000000000000005"}}}"#; + let expected = r#"{"id":"0xf","payload":{"sign":{"address":"0x0000000000000000000000000000000000000001","data":"0x05"}}}"#; // then assert_eq!(res.unwrap(), expected.to_owned());