Skip to content

Expose event message headers, introduce a new way to read the body from an io.Reader#1955

Merged
gmlewis merged 7 commits into
google:masterfrom
m19c:master
Jul 13, 2021
Merged

Expose event message headers, introduce a new way to read the body from an io.Reader#1955
gmlewis merged 7 commits into
google:masterfrom
m19c:master

Conversation

@m19c
Copy link
Copy Markdown
Contributor

@m19c m19c commented Jul 8, 2021

This pull request introduces the changes described in #1944.

  • Expose the constants sha1SignatureHeader, sha256SignatureHeader, eventTypeHeader and deliveryIDHeader
  • Introduce a new public function called ValidatePayloadFromBody to read the body directly from an io.Reader rather than a http.Request.

The changes described enables the possibility to use go-github even with http libraries like fiber or fasthttp.

Example A:

app.Get(func(ctx *fiber.Ctx) error {
	signature := ctx.Get(github.SHA256SignatureHeader)
	if signature == "" {
		signature = ctx.Get(github.SHA1SignatureHeader)
	}
	
	payload := ctx.Body()
	err := github.ValidateSignature(signature, payload, mySecretToken)
	// ...

	event, err := github.ParseWebHook(ctx.Get(github.EventTypeHeader), payload)
	// ...
})

Example B:

app.Get(func(ctx *fiber.Ctx) error {
	signature := ctx.Get(github.SHA256SignatureHeader)
	if signature == "" {
		signature = ctx.Get(github.SHA1SignatureHeader)
	}
	
	payload, err := github.ValidatePayloadFromBody(ctx.Get("Content-Type"), ctx.Context().RequestBodyStream(), signature, mySecretToken)
	// ...

	event, err := github.ParseWebHook(ctx.Get(github.EventTypeHeader), payload)
	// ...
})

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Jul 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla Bot added the cla: no label Jul 8, 2021
@m19c
Copy link
Copy Markdown
Contributor Author

m19c commented Jul 8, 2021

@googlebot I signed it!

@google-cla google-cla Bot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Jul 8, 2021
@m19c m19c changed the title Expose event message headers, introduce a new ay to read the body from an io.Reader Expose event message headers, introduce a new way to read the body from an io.Reader Jul 8, 2021
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @m19c !
LGTM.

Awaiting second LGTM before merging.

@gmlewis gmlewis requested a review from wesleimp July 8, 2021 20:08
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 8, 2021

Codecov Report

Merging #1955 (329d1c9) into master (a7a3d8c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1955      +/-   ##
==========================================
+ Coverage   97.84%   97.87%   +0.02%     
==========================================
  Files         105      105              
  Lines        6809     6809              
==========================================
+ Hits         6662     6664       +2     
+ Misses         80       79       -1     
+ Partials       67       66       -1     
Impacted Files Coverage Δ
github/messages.go 100.00% <100.00%> (+2.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7a3d8c...329d1c9. Read the comment docs.

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jul 8, 2021

@m19c - do you want to resolve the merge conflicts, or would you like me to do that?

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jul 8, 2021

Thank you, @m19c ! Could you please run gofmt one more time and push the changes to make the tests pass?

@m19c
Copy link
Copy Markdown
Contributor Author

m19c commented Jul 8, 2021

Done. Intent should be fixed now.

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jul 8, 2021

@m19c - bonus points if you can increase the code coverage by writing a test for your new method (while we are waiting for a second LGTM). 😄

@m19c
Copy link
Copy Markdown
Contributor Author

m19c commented Jul 8, 2021

Done 👍🏽

@m19c
Copy link
Copy Markdown
Contributor Author

m19c commented Jul 8, 2021

I've also added another test to capture invalid application/x-www-form-urlencoded requests.
The messages.go file now has 100% test coverage.

Copy link
Copy Markdown

@Parker77 Parker77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@gmlewis gmlewis merged commit 7af5768 into google:master Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants