From 4f6c581fc9e344c1cc918e14e632d3c2dd93c2de Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Fri, 17 Aug 2018 15:23:46 +0200 Subject: [PATCH] signer: cliui formatting, remove local path disclosure, update extapi docs --- cmd/clef/extapi_changelog.md | 5 +++++ cmd/clef/intapi_changelog.md | 4 ++++ cmd/clef/main.go | 2 +- signer/core/api.go | 29 ++++++++++++++--------------- signer/core/api_test.go | 28 ++++++++++++++-------------- signer/core/auditlog.go | 10 ++++------ signer/core/cliui.go | 21 ++++++++++++--------- signer/core/stdioui.go | 4 ++-- signer/rules/rules.go | 6 +++--- signer/rules/rules_test.go | 23 +++++++++-------------- 10 files changed, 68 insertions(+), 64 deletions(-) diff --git a/cmd/clef/extapi_changelog.md b/cmd/clef/extapi_changelog.md index 2014e90ae4..3629727b00 100644 --- a/cmd/clef/extapi_changelog.md +++ b/cmd/clef/extapi_changelog.md @@ -1,6 +1,11 @@ ### Changelog for external API +### 3.0.0 + +* The external `accounts_List`-method was changed to not expose `url`, which contained information about the local machine. It now returns a set of addresses: `[]common.Address`. +* The method was also renamed into `accounts_listAccounts`. + #### 2.0.0 diff --git a/cmd/clef/intapi_changelog.md b/cmd/clef/intapi_changelog.md index 7d2a897ea2..aa7087cd0c 100644 --- a/cmd/clef/intapi_changelog.md +++ b/cmd/clef/intapi_changelog.md @@ -1,5 +1,9 @@ ### Changelog for internal API (ui-api) +### 3.0.0 + +* The external `accounts_List`-method was changed to not expose `url`, which contained information about the local machine. Thus, the internal method `approveListing` was changed correspondingly, to contain only a set of addresses: `[]common.Address` + ### 2.0.0 * Modify how `call_info` on a transaction is conveyed. New format: diff --git a/cmd/clef/main.go b/cmd/clef/main.go index 85704754de..4b44a9d1df 100644 --- a/cmd/clef/main.go +++ b/cmd/clef/main.go @@ -591,7 +591,7 @@ func testExternalUI(api *core.SignerAPI) { checkErr("SignTransaction", err) _, err = api.Sign(ctx, common.MixedcaseAddress{}, common.Hex2Bytes("01020304")) checkErr("Sign", err) - _, err = api.List(ctx) + _, err = api.ListAccounts(ctx) checkErr("List", err) _, err = api.New(ctx) checkErr("New", err) diff --git a/signer/core/api.go b/signer/core/api.go index 9a2a49ccc6..8dd39570d3 100644 --- a/signer/core/api.go +++ b/signer/core/api.go @@ -38,8 +38,8 @@ import ( // ExternalAPI defines the external API through which signing requests are made. type ExternalAPI interface { - // List available accounts - List(ctx context.Context) (Accounts, error) + // ListAccounts lists available accounts (addresses) + ListAccounts(ctx context.Context) ([]common.Address, error) // New request to create a new account New(ctx context.Context) (accounts.Account, error) // SignTransaction request to sign the specified transaction @@ -66,7 +66,7 @@ type SignerUI interface { ApproveImport(request *ImportRequest) (ImportResponse, error) // ApproveListing prompt the user for confirmation to list accounts // the list of accounts to list can be modified by the UI - ApproveListing(request *ListRequest) (ListResponse, error) + ApproveListing(request *ListAccountsRequest) (ListAccountsResponse, error) // ApproveNewAccount prompt the user for confirmation to create new Account, and reveal to caller ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) // ShowError displays error message to user @@ -172,12 +172,12 @@ type ( Approved bool `json:"approved"` Password string `json:"password"` } - ListRequest struct { - Accounts []Account `json:"accounts"` - Meta Metadata `json:"meta"` + ListAccountsRequest struct { + Accounts []common.Address `json:"accounts"` + Meta Metadata `json:"meta"` } - ListResponse struct { - Accounts []Account `json:"accounts"` + ListAccountsResponse struct { + Accounts []common.Address `json:"accounts"` } Message struct { Text string `json:"text"` @@ -225,17 +225,16 @@ func NewSignerAPI(chainID int64, ksLocation string, noUSB bool, ui SignerUI, abi return &SignerAPI{big.NewInt(chainID), accounts.NewManager(backends...), ui, NewValidator(abidb)} } -// List returns the set of wallet this signer manages. Each wallet can contain +// ListAccounts returns the set of wallet this signer manages. Each wallet can contain // multiple accounts. -func (api *SignerAPI) List(ctx context.Context) (Accounts, error) { - var accs []Account +func (api *SignerAPI) ListAccounts(ctx context.Context) ([]common.Address, error) { + addresses := make([]common.Address, 0) // return [] instead of nil if empty for _, wallet := range api.am.Wallets() { - for _, acc := range wallet.Accounts() { - acc := Account{Typ: "Account", URL: wallet.URL(), Address: acc.Address} - accs = append(accs, acc) + for _, account := range wallet.Accounts() { + addresses = append(addresses, account.Address) } } - result, err := api.UI.ApproveListing(&ListRequest{Accounts: accs, Meta: MetadataFromContext(ctx)}) + result, err := api.UI.ApproveListing(&ListAccountsRequest{Accounts: addresses, Meta: MetadataFromContext(ctx)}) if err != nil { return nil, err } diff --git a/signer/core/api_test.go b/signer/core/api_test.go index 50ad021987..3b013fac58 100644 --- a/signer/core/api_test.go +++ b/signer/core/api_test.go @@ -80,17 +80,17 @@ func (ui *HeadlessUI) ApproveImport(request *ImportRequest) (ImportResponse, err } return ImportResponse{false, "", ""}, nil } -func (ui *HeadlessUI) ApproveListing(request *ListRequest) (ListResponse, error) { +func (ui *HeadlessUI) ApproveListing(request *ListAccountsRequest) (ListAccountsResponse, error) { switch <-ui.controller { case "A": - return ListResponse{request.Accounts}, nil + return ListAccountsResponse{request.Accounts}, nil case "1": - l := make([]Account, 1) + l := make([]common.Address, 1) l[0] = request.Accounts[1] - return ListResponse{l}, nil + return ListAccountsResponse{l}, nil default: - return ListResponse{nil}, nil + return ListAccountsResponse{nil}, nil } } func (ui *HeadlessUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) { @@ -162,9 +162,9 @@ func failCreateAccount(control chan string, api *SignerAPI, t *testing.T) { t.Fatal("Empty address should be returned") } } -func list(control chan string, api *SignerAPI, t *testing.T) []Account { +func list(control chan string, api *SignerAPI, t *testing.T) []common.Address { control <- "A" - list, err := api.List(context.Background()) + list, err := api.ListAccounts(context.Background()) if err != nil { t.Fatal(err) } @@ -193,7 +193,7 @@ func TestNewAcc(t *testing.T) { // Testing listing: // Listing one Account control <- "1" - list, err := api.List(context.Background()) + list, err := api.ListAccounts(context.Background()) if err != nil { t.Fatal(err) } @@ -202,7 +202,7 @@ func TestNewAcc(t *testing.T) { } // Listing denied control <- "Nope" - list, err = api.List(context.Background()) + list, err = api.ListAccounts(context.Background()) if len(list) != 0 { t.Fatalf("List should be empty") } @@ -218,11 +218,11 @@ func TestSignData(t *testing.T) { createAccount(control, api, t) createAccount(control, api, t) control <- "1" - list, err := api.List(context.Background()) + list, err := api.ListAccounts(context.Background()) if err != nil { t.Fatal(err) } - a := common.NewMixedcaseAddress(list[0].Address) + a := common.NewMixedcaseAddress(list[0]) control <- "Y" control <- "wrongpassword" @@ -275,7 +275,7 @@ func mkTestTx(from common.MixedcaseAddress) SendTxArgs { func TestSignTx(t *testing.T) { var ( - list Accounts + list []common.Address res, res2 *ethapi.SignTransactionResult err error ) @@ -283,11 +283,11 @@ func TestSignTx(t *testing.T) { api, control := setup(t) createAccount(control, api, t) control <- "A" - list, err = api.List(context.Background()) + list, err = api.ListAccounts(context.Background()) if err != nil { t.Fatal(err) } - a := common.NewMixedcaseAddress(list[0].Address) + a := common.NewMixedcaseAddress(list[0]) methodSig := "test(uint)" tx := mkTestTx(a) diff --git a/signer/core/auditlog.go b/signer/core/auditlog.go index d0ba733d2f..6ba99d4497 100644 --- a/signer/core/auditlog.go +++ b/signer/core/auditlog.go @@ -33,12 +33,10 @@ type AuditLogger struct { api ExternalAPI } -func (l *AuditLogger) List(ctx context.Context) (Accounts, error) { - l.log.Info("List", "type", "request", "metadata", MetadataFromContext(ctx).String()) - res, e := l.api.List(ctx) - - l.log.Info("List", "type", "response", "data", res.String()) - +func (l *AuditLogger) ListAccounts(ctx context.Context) ([]common.Address, error) { + l.log.Info("ListAccounts", "type", "request", "metadata", MetadataFromContext(ctx).String()) + res, e := l.api.ListAccounts(ctx) + l.log.Info("ListAccounts", "type", "response", "data", res) return res, e } diff --git a/signer/core/cliui.go b/signer/core/cliui.go index 2f969669c2..29387eeb92 100644 --- a/signer/core/cliui.go +++ b/signer/core/cliui.go @@ -109,20 +109,23 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, erro fmt.Printf("\nWARNING: Invalid checksum on to-address!\n\n") } } else { - fmt.Printf("to: \n") + fmt.Printf("To: \n") } - fmt.Printf("from: %v\n", request.Transaction.From.String()) - fmt.Printf("value: %v wei\n", weival) + fmt.Printf("Trom: %v\n", request.Transaction.From.String()) + fmt.Printf("Value: %v wei\n", weival) + fmt.Printf("Gas: %v\n", request.Transaction.Gas) + fmt.Printf("Gasprice: %v wei\n", request.Transaction.GasPrice.ToInt()) + fmt.Printf("Nonce: %v\n", request.Transaction.Nonce) if request.Transaction.Data != nil { d := *request.Transaction.Data if len(d) > 0 { - fmt.Printf("data: %v\n", common.Bytes2Hex(d)) + fmt.Printf("Data: %v\n", common.Bytes2Hex(d)) } } if request.Callinfo != nil { fmt.Printf("\nTransaction validation:\n") for _, m := range request.Callinfo { - fmt.Printf(" * %s : %s", m.Typ, m.Message) + fmt.Printf(" * %s : %s\n", m.Typ, m.Message) } fmt.Println() @@ -187,7 +190,7 @@ func (ui *CommandlineUI) ApproveImport(request *ImportRequest) (ImportResponse, // ApproveListing prompt the user for confirmation to list accounts // the list of accounts to list can be modified by the UI -func (ui *CommandlineUI) ApproveListing(request *ListRequest) (ListResponse, error) { +func (ui *CommandlineUI) ApproveListing(request *ListAccountsRequest) (ListAccountsResponse, error) { ui.mu.Lock() defer ui.mu.Unlock() @@ -196,14 +199,14 @@ func (ui *CommandlineUI) ApproveListing(request *ListRequest) (ListResponse, err fmt.Printf("A request has been made to list all accounts. \n") fmt.Printf("You can select which accounts the caller can see\n") for _, account := range request.Accounts { - fmt.Printf("\t[x] %v\n", account.Address.Hex()) + fmt.Printf("\t[x] %v\n", account.Hex()) } fmt.Printf("-------------------------------------------\n") showMetadata(request.Meta) if !ui.confirm() { - return ListResponse{nil}, nil + return ListAccountsResponse{nil}, nil } - return ListResponse{request.Accounts}, nil + return ListAccountsResponse{request.Accounts}, nil } // ApproveNewAccount prompt the user for confirmation to create new Account, and reveal to caller diff --git a/signer/core/stdioui.go b/signer/core/stdioui.go index 5640ed03bd..1193f997d8 100644 --- a/signer/core/stdioui.go +++ b/signer/core/stdioui.go @@ -73,8 +73,8 @@ func (ui *StdIOUI) ApproveImport(request *ImportRequest) (ImportResponse, error) return result, err } -func (ui *StdIOUI) ApproveListing(request *ListRequest) (ListResponse, error) { - var result ListResponse +func (ui *StdIOUI) ApproveListing(request *ListAccountsRequest) (ListAccountsResponse, error) { + var result ListAccountsResponse err := ui.dispatch("ApproveListing", request, &result) return result, err } diff --git a/signer/rules/rules.go b/signer/rules/rules.go index 711e2dddec..d5757dccad 100644 --- a/signer/rules/rules.go +++ b/signer/rules/rules.go @@ -194,7 +194,7 @@ func (r *rulesetUI) ApproveImport(request *core.ImportRequest) (core.ImportRespo return r.next.ApproveImport(request) } -func (r *rulesetUI) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { +func (r *rulesetUI) ApproveListing(request *core.ListAccountsRequest) (core.ListAccountsResponse, error) { jsonreq, err := json.Marshal(request) approved, err := r.checkApproval("ApproveListing", jsonreq, err) if err != nil { @@ -202,9 +202,9 @@ func (r *rulesetUI) ApproveListing(request *core.ListRequest) (core.ListResponse return r.next.ApproveListing(request) } if approved { - return core.ListResponse{Accounts: request.Accounts}, nil + return core.ListAccountsResponse{Accounts: request.Accounts}, nil } - return core.ListResponse{}, err + return core.ListAccountsResponse{}, err } func (r *rulesetUI) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) { diff --git a/signer/rules/rules_test.go b/signer/rules/rules_test.go index b6060eba73..2e3e850d05 100644 --- a/signer/rules/rules_test.go +++ b/signer/rules/rules_test.go @@ -22,7 +22,6 @@ import ( "strings" "testing" - "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/core/types" @@ -93,8 +92,8 @@ func (alwaysDenyUI) ApproveImport(request *core.ImportRequest) (core.ImportRespo return core.ImportResponse{Approved: false, OldPassword: "", NewPassword: ""}, nil } -func (alwaysDenyUI) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { - return core.ListResponse{Accounts: nil}, nil +func (alwaysDenyUI) ApproveListing(request *core.ListAccountsRequest) (core.ListAccountsResponse, error) { + return core.ListAccountsResponse{Accounts: nil}, nil } func (alwaysDenyUI) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) { @@ -125,15 +124,11 @@ func initRuleEngine(js string) (*rulesetUI, error) { } func TestListRequest(t *testing.T) { - accs := make([]core.Account, 5) + accs := make([]common.Address, 5) for i := range accs { addr := fmt.Sprintf("000000000000000000000000000000000000000%x", i) - acc := core.Account{ - Address: common.BytesToAddress(common.Hex2Bytes(addr)), - URL: accounts.URL{Scheme: "test", Path: fmt.Sprintf("acc-%d", i)}, - } - accs[i] = acc + accs[i] = common.BytesToAddress(common.Hex2Bytes(addr)) } js := `function ApproveListing(){ return "Approve" }` @@ -143,7 +138,7 @@ func TestListRequest(t *testing.T) { t.Errorf("Couldn't create evaluator %v", err) return } - resp, err := r.ApproveListing(&core.ListRequest{ + resp, err := r.ApproveListing(&core.ListAccountsRequest{ Accounts: accs, Meta: core.Metadata{Remote: "remoteip", Local: "localip", Scheme: "inproc"}, }) @@ -220,9 +215,9 @@ func (d *dummyUI) ApproveImport(request *core.ImportRequest) (core.ImportRespons return core.ImportResponse{}, core.ErrRequestDenied } -func (d *dummyUI) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { +func (d *dummyUI) ApproveListing(request *core.ListAccountsRequest) (core.ListAccountsResponse, error) { d.calls = append(d.calls, "ApproveListing") - return core.ListResponse{}, core.ErrRequestDenied + return core.ListAccountsResponse{}, core.ErrRequestDenied } func (d *dummyUI) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) { @@ -532,9 +527,9 @@ func (d *dontCallMe) ApproveImport(request *core.ImportRequest) (core.ImportResp return core.ImportResponse{}, core.ErrRequestDenied } -func (d *dontCallMe) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { +func (d *dontCallMe) ApproveListing(request *core.ListAccountsRequest) (core.ListAccountsResponse, error) { d.t.Fatalf("Did not expect next-handler to be called") - return core.ListResponse{}, core.ErrRequestDenied + return core.ListAccountsResponse{}, core.ErrRequestDenied } func (d *dontCallMe) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) {