Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor verifySignature for re-use #721

Merged
merged 4 commits into from
Nov 8, 2018
Merged

Conversation

lukebjerring
Copy link
Contributor

@lukebjerring lukebjerring commented Nov 2, 2018

Description

Extract the "load secret + verify payload" behaviour into a reusable method, in prep for adding another GitHub webhook for custom checks.

Working towards #712

@lukebjerring lukebjerring requested a review from Hexcles November 2, 2018 19:03
@wpt-pr-bot
Copy link

Staging instance deployed by Travis CI!
Running at https://verify-signature-dot-wptdashboard-staging.appspot.com

Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

Two nits.

(Sorry I forgot to send the draft last Friday...)

api/webhook/verify.go Outdated Show resolved Hide resolved
api/webhook/verify.go Outdated Show resolved Hide resolved
@wpt-pr-bot
Copy link

Staging instance deployed by Travis CI!
Running at https://verify-signature-dot-announcer-dot-wptdashboard-staging.appspot.com

Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

LGTM

"github.com/web-platform-tests/wpt.fyi/shared"
)

func verifyAndGetPayload(r *http.Request) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Optional: this is fine for now, but I imagine you'd like to reuse this method in checks to verify the payload? (I saw your TODO in that PR.)

When that happens, we'd need to export (and perhaps move) this function, and add tokenName as an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do :)

@lukebjerring lukebjerring merged commit a780392 into master Nov 8, 2018
@lukebjerring lukebjerring deleted the verify-signature branch November 8, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants