R4R: validate sign tx request's body #3179

alessio · 2018-12-20T14:50:06Z

Closes: #3176

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alexanderbez

Thanks @alessio -- a few minor remarks ^__^

client/utils/rest.go

x/auth/client/rest/sign.go

codecov · 2018-12-20T14:57:59Z

Codecov Report

Merging #3179 into develop will increase coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3179      +/-   ##
===========================================
+ Coverage    54.72%   54.73%   +<.01%     
===========================================
  Files          132      132              
  Lines         9425     9427       +2     
===========================================
+ Hits          5158     5160       +2     
  Misses        3949     3949              
  Partials       318      318

codecov · 2018-12-20T14:57:59Z

Codecov Report

Merging #3179 into develop will increase coverage by 0.01%.
The diff coverage is 20%.

@@             Coverage Diff             @@
##           develop    #3179      +/-   ##
===========================================
+ Coverage    54.83%   54.84%   +0.01%     
===========================================
  Files          133      133              
  Lines         9559     9557       -2     
===========================================
  Hits          5242     5242              
+ Misses        3996     3994       -2     
  Partials       321      321

fedekunze · 2018-12-20T14:54:54Z

client/lcd/lcd_test.go

+		BaseReq: utils.BaseReq{
+			Name:          name1,
+			Password:      pw,
+			ChainID:       viper.GetString(client.FlagChainID),


why are we using a flag on the REST tests ?

You may find the answer in InitializeTestLCD()

client/lcd/lcd_test.go

client/lcd/test_helpers.go

fedekunze · 2018-12-20T14:56:48Z

client/utils/rest.go

@@ -189,7 +189,7 @@ func ReadRESTReq(w http.ResponseWriter, r *http.Request, cdc *codec.Codec, req i

 	err = cdc.UnmarshalJSON(body, req)
 	if err != nil {
-		WriteErrorResponse(w, http.StatusBadRequest, err.Error())
+		WriteErrorResponse(w, http.StatusBadRequest, "failed to decode JSON payload: "+err.Error())


@alexanderbez your request was only for signing or for all of the unmarshal errors ?

fedekunze · 2018-12-20T14:58:06Z

x/auth/client/rest/sign.go

-	AppendSig        bool       `json:"append_sig"`
+	Tx        auth.StdTx `json:"tx"`
+	AppendSig bool       `json:"append_sig"`
+	utils.BaseReq


add BaseReq and json:"base_req"

why? that would break the interface. Do we really need a nested object?

were using it like that everywhere on the POST requests. But maybe we need to consider removing it. Thoughts @alexanderbez @jackzampolin @faboweb ?

In that case, I'm fine with either

Hmm, yes. So, me personally, I'd rather avoid having to use base_req. These are all primary/first-class-citizen fields of the request and not some inner resource, so I don't think it should be nested.

tl;dr I think we should leave this as-is and update the other REST requests (breaking, sorry I know).

hmmm, currently the standard is to have base_req. I vote to keep this here to the standard and open an issue about changing this in all request to first class citizens. This should then probably also be discussed with others.
side note: I actually think this structure makes sense as it decouples the content and the meta information.

I'd also have the base_req for consistency, but I don't have a strong opinion on it since we weren't using it before on this request

x/auth/client/rest/sign.go

alexanderbez

utACK ☕️

jackzampolin · 2018-12-21T17:09:46Z

I think this requires a swagger.yaml update. Can you please add that @alessio

rigelrozanski

Pretty straight forward changes

rigelrozanski · 2018-12-27T06:06:44Z

client/lcd/lcd_test.go

-		Sequence:         sequence,
+		Tx: msg,
+		BaseReq: utils.NewBaseReq(
+			name1, pw, "", viper.GetString(client.FlagChainID), "", "", accnum, sequence, nil, false, false,


rigelrozanski · 2018-12-27T06:10:29Z

x/auth/client/rest/sign.go

+		}
+
+		// validate tx
+		// discard error if it's CodeUnauthorized as the tx comes with no signatures


I'm a little bit confused as to why CodeUnautherized implies that there tx comes with no signatures... maybe we should have CodeNoSignatures? Don't deal with this code a lot just an outsider's perspective

CodeNoSignatures sounds good and self-explanatory to me 👍

Amended, please review again

Closes: #3176

alessio · 2019-01-02T10:09:46Z

@jackzampolin changed applied, please review

jackzampolin

Thanks @alessio, this works for me locally!

x/auth/client/rest/sign.go

Co-Authored-By: alessio <quadrispro@ubuntu.com>

alessio requested review from alexanderbez, jackzampolin and fedekunze December 20, 2018 14:50

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners December 20, 2018 14:50

alexanderbez suggested changes Dec 20, 2018

View reviewed changes

client/utils/rest.go Outdated Show resolved Hide resolved

x/auth/client/rest/sign.go Outdated Show resolved Hide resolved

x/auth/client/rest/sign.go Outdated Show resolved Hide resolved

fedekunze reviewed Dec 20, 2018

View reviewed changes

alexanderbez approved these changes Dec 20, 2018

View reviewed changes

alessio mentioned this pull request Dec 21, 2018

Flatten REST POST payload #3188

Closed

4 tasks

rigelrozanski reviewed Dec 27, 2018

View reviewed changes

rigelrozanski changed the title ~~validate sign tx request's body~~ R4R validate sign tx request's body Dec 27, 2018

rigelrozanski added the ready-for-review label Dec 27, 2018

validate sign tx request's body

1ebe184

Closes: #3176

alessio force-pushed the alessio/3176-validate-sign-body-req branch from e342a32 to 1ebe184 Compare December 27, 2018 13:43

Alessio Treglia added 2 commits January 2, 2019 10:05

Introduce CodeNoSignatures

11585a4

Update swagger.yaml

903348a

Fix tests

d78db91

jackzampolin approved these changes Jan 2, 2019

View reviewed changes

rigelrozanski reviewed Jan 2, 2019

View reviewed changes

x/auth/client/rest/sign.go Outdated Show resolved Hide resolved

Update x/auth/client/rest/sign.go

5448824

Co-Authored-By: alessio <quadrispro@ubuntu.com>

alessio changed the title ~~R4R validate sign tx request's body~~ R4R: validate sign tx request's body Jan 2, 2019

jackzampolin merged commit d1824ad into develop Jan 3, 2019

jackzampolin deleted the alessio/3176-validate-sign-body-req branch January 3, 2019 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: validate sign tx request's body #3179

R4R: validate sign tx request's body #3179

alessio commented Dec 20, 2018 •

edited

Loading

alexanderbez left a comment

codecov bot commented Dec 20, 2018

codecov bot commented Dec 20, 2018 •

edited

Loading

fedekunze Dec 20, 2018

alessio Dec 20, 2018

fedekunze Dec 20, 2018

fedekunze Dec 20, 2018

alessio Dec 20, 2018

fedekunze Dec 20, 2018

alessio Dec 20, 2018

alexanderbez Dec 20, 2018

faboweb Dec 21, 2018 •

edited

Loading

fedekunze Dec 21, 2018

alexanderbez left a comment

jackzampolin commented Dec 21, 2018

rigelrozanski left a comment

rigelrozanski Dec 27, 2018

rigelrozanski Dec 27, 2018

alessio Dec 27, 2018

alessio Jan 2, 2019

alessio commented Jan 2, 2019

jackzampolin left a comment

R4R: validate sign tx request's body #3179

R4R: validate sign tx request's body #3179

Conversation

alessio commented Dec 20, 2018 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2018

Codecov Report

codecov bot commented Dec 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faboweb Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

jackzampolin commented Dec 21, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Jan 2, 2019

jackzampolin left a comment

Choose a reason for hiding this comment

alessio commented Dec 20, 2018 •

edited

Loading

codecov bot commented Dec 20, 2018 •

edited

Loading

faboweb Dec 21, 2018 •

edited

Loading