From 7718476cafcd6b330bdd15ae02e5ab16ef6ffe9b Mon Sep 17 00:00:00 2001 From: Jamie Holdstock Date: Fri, 17 Feb 2023 20:44:13 +0000 Subject: [PATCH] webapi: Move VSP closed check to middleware. Checking this in middleware not only removes duplicated code from each of the handlers, but also makes things more efficient. Previously, dcrd/dcrwallet/database clients were initialized (and could possibly cause errors) **before** checking if the VSP is closed. Now the check happens before those other things. Also bundled with some improvements to setaltsignaddr_test.go which I implemented along the way. --- webapi/getfeeaddress.go | 5 - webapi/middleware.go | 7 ++ webapi/payfee.go | 5 - webapi/setaltsignaddr.go | 5 - webapi/setaltsignaddr_test.go | 191 ++++++++++++++++++++-------------- webapi/webapi.go | 6 +- 6 files changed, 123 insertions(+), 96 deletions(-) diff --git a/webapi/getfeeaddress.go b/webapi/getfeeaddress.go index 5e76286..9d04977 100644 --- a/webapi/getfeeaddress.go +++ b/webapi/getfeeaddress.go @@ -90,11 +90,6 @@ func (s *Server) feeAddress(c *gin.Context) { } reqBytes := c.MustGet(requestBytesKey).([]byte) - if s.cfg.VspClosed { - s.sendError(types.ErrVspClosed, c) - return - } - var request types.FeeAddressRequest if err := binding.JSON.BindBody(reqBytes, &request); err != nil { s.log.Warnf("%s: Bad request (clientIP=%s): %v", funcName, c.ClientIP(), err) diff --git a/webapi/middleware.go b/webapi/middleware.go index c15ed83..3a47a0b 100644 --- a/webapi/middleware.go +++ b/webapi/middleware.go @@ -111,6 +111,13 @@ func drainAndReplaceBody(req *http.Request) ([]byte, error) { return reqBytes, nil } +func (s *Server) vspMustBeOpen(c *gin.Context) { + if s.cfg.VspClosed { + s.sendError(types.ErrVspClosed, c) + return + } +} + // broadcastTicket will ensure that the local dcrd instance is aware of the // provided ticket. // Ticket hash, ticket hex, and parent hex are parsed from the request body and diff --git a/webapi/payfee.go b/webapi/payfee.go index 548c6e2..096e2bf 100644 --- a/webapi/payfee.go +++ b/webapi/payfee.go @@ -36,11 +36,6 @@ func (s *Server) payFee(c *gin.Context) { } reqBytes := c.MustGet(requestBytesKey).([]byte) - if s.cfg.VspClosed { - s.sendError(types.ErrVspClosed, c) - return - } - if !knownTicket { s.log.Warnf("%s: Unknown ticket (clientIP=%s)", funcName, c.ClientIP()) s.sendError(types.ErrUnknownTicket, c) diff --git a/webapi/setaltsignaddr.go b/webapi/setaltsignaddr.go index ad7bada..5aaef59 100644 --- a/webapi/setaltsignaddr.go +++ b/webapi/setaltsignaddr.go @@ -40,11 +40,6 @@ func (s *Server) setAltSignAddr(c *gin.Context) { } reqBytes := c.MustGet(requestBytesKey).([]byte) - if s.cfg.VspClosed { - s.sendError(types.ErrVspClosed, c) - return - } - var request types.SetAltSignAddrRequest if err := binding.JSON.BindBody(reqBytes, &request); err != nil { s.log.Warnf("%s: Bad request (clientIP=%s): %v", funcName, c.ClientIP(), err) diff --git a/webapi/setaltsignaddr_test.go b/webapi/setaltsignaddr_test.go index 773a5ba..b57b6b7 100644 --- a/webapi/setaltsignaddr_test.go +++ b/webapi/setaltsignaddr_test.go @@ -10,6 +10,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "math/rand" "net/http" "net/http/httptest" @@ -130,87 +131,100 @@ func (n *testNode) GetRawTransaction(txHash string) (*dcrdtypes.TxRawResult, err func TestSetAltSignAddress(t *testing.T) { const testAddr = "DsVoDXNQqyF3V83PJJ5zMdnB4pQuJHBAh15" - tests := []struct { - name string + tests := map[string]struct { dcrdClientErr bool - vspClosed bool deformReq int addr string node *testNode isExistingAltSignAddr bool - wantCode int - }{{ - name: "ok", - addr: testAddr, - node: &testNode{ - getRawTransaction: &dcrdtypes.TxRawResult{ - Confirmations: 1000, + wantHTTPStatus int + // wantErrCode and wantErrMsg only checked if wantHTTPStatus != 200. + wantErrCode types.ErrorCode + wantErrMsg string + }{ + "ok": { + addr: testAddr, + node: &testNode{ + getRawTransaction: &dcrdtypes.TxRawResult{ + Confirmations: 1000, + }, + getRawTransactionErr: nil, + existsLiveTicket: true, }, - getRawTransactionErr: nil, - existsLiveTicket: true, + wantHTTPStatus: http.StatusOK, }, - wantCode: http.StatusOK, - }, { - name: "vsp closed", - vspClosed: true, - wantCode: http.StatusBadRequest, - }, { - name: "dcrd client error", - dcrdClientErr: true, - wantCode: http.StatusInternalServerError, - }, { + "dcrd client error": { + dcrdClientErr: true, + wantHTTPStatus: http.StatusInternalServerError, + wantErrCode: types.ErrInternalError, + wantErrMsg: types.ErrInternalError.DefaultMessage(), + }, + "bad request": { + deformReq: 1, + wantHTTPStatus: http.StatusBadRequest, + wantErrCode: types.ErrBadRequest, + wantErrMsg: "json: cannot unmarshal string into Go value of type types.SetAltSignAddrRequest", + }, + "bad addr": { + addr: "xxx", + wantHTTPStatus: http.StatusBadRequest, + wantErrCode: types.ErrBadRequest, + wantErrMsg: "failed to decoded address \"xxx\": invalid format: version and/or checksum bytes missing", + }, + "addr wrong type": { + addr: "DkM3ZigNyiwHrsXRjkDQ8t8tW6uKGW9g61qEkG3bMqQPQWYEf5X3J", + wantHTTPStatus: http.StatusBadRequest, + wantErrCode: types.ErrBadRequest, + wantErrMsg: "wrong type for alternate signing address", + }, + "getRawTransaction error from dcrd client": { + addr: testAddr, + node: &testNode{ + getRawTransactionErr: errors.New("getRawTransaction error"), + }, + wantHTTPStatus: http.StatusInternalServerError, + wantErrCode: types.ErrInternalError, + wantErrMsg: types.ErrInternalError.DefaultMessage(), + }, + "existsLiveTicket error from dcrd client": { + addr: testAddr, + node: &testNode{ + getRawTransaction: &dcrdtypes.TxRawResult{ + Confirmations: 1000, + }, + existsLiveTicketErr: errors.New("existsLiveTicket error"), + }, + wantHTTPStatus: http.StatusInternalServerError, + wantErrCode: types.ErrInternalError, + wantErrMsg: types.ErrInternalError.DefaultMessage(), + }, + "ticket can't vote": { + addr: testAddr, + node: &testNode{ + getRawTransaction: &dcrdtypes.TxRawResult{ + Confirmations: 1000, + }, + existsLiveTicket: false, + }, + wantHTTPStatus: http.StatusBadRequest, + wantErrCode: types.ErrTicketCannotVote, + wantErrMsg: types.ErrTicketCannotVote.DefaultMessage(), + }, + "only one alt sign addr allowed": { + addr: testAddr, + node: &testNode{ + getRawTransaction: &dcrdtypes.TxRawResult{}, + existsLiveTicket: true, + }, + isExistingAltSignAddr: true, + wantHTTPStatus: http.StatusBadRequest, + wantErrCode: types.ErrBadRequest, + wantErrMsg: "alternate sign address data already exists", + }, + } - name: "bad request", - deformReq: 1, - wantCode: http.StatusBadRequest, - }, { - name: "bad addr", - addr: "xxx", - wantCode: http.StatusBadRequest, - }, { - name: "addr wrong type", - addr: "DkM3ZigNyiwHrsXRjkDQ8t8tW6uKGW9g61qEkG3bMqQPQWYEf5X3J", - wantCode: http.StatusBadRequest, - }, { - name: "getRawTransaction error from dcrd client", - addr: testAddr, - node: &testNode{ - getRawTransactionErr: errors.New("getRawTransaction error"), - }, - wantCode: http.StatusInternalServerError, - }, { - name: "existsLiveTicket error from dcrd client", - addr: testAddr, - node: &testNode{ - getRawTransaction: &dcrdtypes.TxRawResult{ - Confirmations: 1000, - }, - existsLiveTicketErr: errors.New("existsLiveTicket error"), - }, - wantCode: http.StatusInternalServerError, - }, { - name: "ticket can't vote", - addr: testAddr, - node: &testNode{ - getRawTransaction: &dcrdtypes.TxRawResult{ - Confirmations: 1000, - }, - existsLiveTicket: false, - }, - wantCode: http.StatusBadRequest, - }, { - name: "only one alt sign addr allowed", - addr: testAddr, - node: &testNode{ - getRawTransaction: &dcrdtypes.TxRawResult{}, - existsLiveTicket: true, - }, - isExistingAltSignAddr: true, - wantCode: http.StatusBadRequest, - }} - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { ticketHash := randString(64, hexCharset) req := &types.SetAltSignAddrRequest{ Timestamp: time.Now().Unix(), @@ -238,8 +252,6 @@ func TestSetAltSignAddress(t *testing.T) { } } - api.cfg.VspClosed = test.vspClosed - w := httptest.NewRecorder() c, r := gin.CreateTestContext(w) @@ -266,8 +278,31 @@ func TestSetAltSignAddress(t *testing.T) { r.ServeHTTP(w, c.Request) - if test.wantCode != w.Code { - t.Fatalf("expected status %d, got %d", test.wantCode, w.Code) + if test.wantHTTPStatus != w.Code { + t.Fatalf("expected http status %d, got %d", test.wantHTTPStatus, w.Code) + } + + if test.wantHTTPStatus != http.StatusOK { + respBytes, err := io.ReadAll(w.Body) + if err != nil { + t.Fatalf("failed reading response body bytes: %v", err) + } + + var apiError types.ErrorResponse + err = json.Unmarshal(respBytes, &apiError) + if err != nil { + t.Fatalf("could not unmarshal error response: %v", err) + } + + if int64(test.wantErrCode) != apiError.Code { + t.Fatalf("incorrect error code, expected %d, actual %d", + test.wantErrCode, apiError.Code) + } + + if test.wantErrMsg != apiError.Message { + t.Fatalf("incorrect error message, expected %q, actual %q", + test.wantErrMsg, apiError.Message) + } } altsig, err := api.db.AltSignAddrData(ticketHash) @@ -275,7 +310,7 @@ func TestSetAltSignAddress(t *testing.T) { t.Fatalf("unable to get alt sign addr data: %v", err) } - if test.wantCode != http.StatusOK && !test.isExistingAltSignAddr { + if test.wantHTTPStatus != http.StatusOK && !test.isExistingAltSignAddr { if altsig != nil { t.Fatalf("expected no alt sign addr saved for errored state") } diff --git a/webapi/webapi.go b/webapi/webapi.go index 9498313..d267ba1 100644 --- a/webapi/webapi.go +++ b/webapi/webapi.go @@ -236,10 +236,10 @@ func (s *Server) router(cookieSecret []byte, dcrd rpc.DcrdConnect, wallets rpc.W api := router.Group("/api/v3") api.GET("/vspinfo", s.vspInfo) - api.POST("/setaltsignaddr", s.withDcrdClient(dcrd), s.broadcastTicket, s.vspAuth, s.setAltSignAddr) - api.POST("/feeaddress", s.withDcrdClient(dcrd), s.broadcastTicket, s.vspAuth, s.feeAddress) + api.POST("/setaltsignaddr", s.vspMustBeOpen, s.withDcrdClient(dcrd), s.broadcastTicket, s.vspAuth, s.setAltSignAddr) + api.POST("/feeaddress", s.vspMustBeOpen, s.withDcrdClient(dcrd), s.broadcastTicket, s.vspAuth, s.feeAddress) api.POST("/ticketstatus", s.withDcrdClient(dcrd), s.vspAuth, s.ticketStatus) - api.POST("/payfee", s.withDcrdClient(dcrd), s.vspAuth, s.payFee) + api.POST("/payfee", s.vspMustBeOpen, s.withDcrdClient(dcrd), s.vspAuth, s.payFee) api.POST("/setvotechoices", s.withDcrdClient(dcrd), s.withWalletClients(wallets), s.vspAuth, s.setVoteChoices) // Website routes.