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

[report] Initial commit of /report and /validate endpoints #31

Open
wants to merge 1 commit into
base: sandbox/joshlf/emulator-unit-test
Choose a base branch
from

Conversation

joshlf
Copy link
Contributor

@joshlf joshlf commented May 8, 2020

This change is Reviewable

@joshlf joshlf requested review from inmyth and madhavajay May 8, 2020 08:57
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 55c872a to 878204e Compare May 8, 2020 09:00
@joshlf joshlf requested a review from cramertj May 8, 2020 18:01
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 878204e to dcd7acc Compare May 8, 2020 20:01
app/internal/report/report.go Outdated Show resolved Hide resolved
app/internal/report/report.go Outdated Show resolved Hide resolved

pendingReportsCollection = "pending_reports"

// 3 days - the period of during which a token is valid and may be verified.
Copy link

Choose a reason for hiding this comment

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

Are these constants inherent to the protocol? Perhaps they should live in a config that can be changed without recompiling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely want to do that eventually, but for prototyping's sake, it's not a priority. If you want to tackle that, though, feel free! #32

app/internal/report/report.go Outdated Show resolved Hide resolved
app/report.go Outdated Show resolved Hide resolved
app/report.go Outdated Show resolved Hide resolved
@joshlf joshlf force-pushed the sandbox/joshlf/golang-4 branch from 2874bab to 5cac050 Compare May 8, 2020 20:35
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from dcd7acc to 1f0c26e Compare May 8, 2020 20:35
@joshlf joshlf changed the base branch from sandbox/joshlf/golang-4 to master May 8, 2020 20:43
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch 4 times, most recently from 4ffa562 to 9af83f4 Compare May 9, 2020 07:27
@joshlf joshlf changed the base branch from master to sandbox/joshlf/emulator-unit-test May 9, 2020 07:28
Copy link
Collaborator

@madhavajay madhavajay left a comment

Choose a reason for hiding this comment

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

Looks good. Are you squashing and force pushing? I guess that would be more clear if I saw this PR earlier.

@joshlf
Copy link
Contributor Author

joshlf commented May 10, 2020

Yeah, I've been spoiled by Gerrit, so I'm trying to figure out how to adapt my workflows to GH.

@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 9af83f4 to 747f99a Compare May 10, 2020 05:38
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 582a7c9 to 4bb9f77 Compare May 10, 2020 05:46
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 747f99a to 08ee308 Compare May 10, 2020 05:46
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 4bb9f77 to f829cc5 Compare May 10, 2020 05:59
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 08ee308 to 684c7e7 Compare May 10, 2020 05:59
@joshlf joshlf changed the title [go][report] Initial commit of /report endpoint [report] Initial commit of /report and /validate endpoints May 10, 2020
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 684c7e7 to ac4e181 Compare May 10, 2020 07:32
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 3ee3327 to f7b6c8f Compare May 10, 2020 08:09
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch 2 times, most recently from cde3e4e to a9e97a6 Compare May 10, 2020 08:12
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 1591bd8 to 90664ea Compare May 10, 2020 22:51
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch 2 times, most recently from 4652316 to 97f0dff Compare May 10, 2020 23:05
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 762c9dc to 49fc05f Compare May 11, 2020 23:45
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 8af8831 to ed574c6 Compare May 12, 2020 04:12
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 49fc05f to 36b3d63 Compare May 12, 2020 04:12
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from ed574c6 to 7d5ba5c Compare May 12, 2020 05:45
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch 3 times, most recently from 7216342 to 5f1cced Compare May 12, 2020 23:38
API.md Outdated Show resolved Hide resolved
API.md Outdated Show resolved Hide resolved
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 5f1cced to 5e18ac8 Compare May 13, 2020 08:08
Copy link
Contributor Author

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @madhavajay)


API.md, line 55 at r1 (raw file):

Previously, madhavajay (Madhava Jay) wrote…

This data payload returns:

 responding with error code 500 and message "internal server error" (error: illegal base64 data at input byte 36)

Changing to this works:

"data" : "9USO+Z30bvZWIKPwZmee0TvkGXBQi7+DqAjtdYZ="

Done.


API.md, line 94 at r1 (raw file):

Previously, madhavajay (Madhava Jay) wrote…

See above:

"data" : "9USO+Z30bvZWIKPwZmee0TvkGXBQi7+DqAjtdYZ="

Done.


functions/internal/report/report.go, line 85 at r1 (raw file):

Previously, madhavajay (Madhava Jay) wrote…

spelling: remvoed -> removed

Done.


functions/internal/report/report_test.go, line 43 at r1 (raw file):

Previously, madhavajay (Madhava Jay) wrote…

number?

Done.

@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 563a290 to c0ba7cb Compare May 13, 2020 09:01
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch 2 times, most recently from 9e6e48b to 55b3b56 Compare May 13, 2020 18:35
Copy link
Collaborator

@madhavajay madhavajay left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @joshlf)


functions/report.go, line 77 at r2 (raw file):

	if req.Challenge != nil {
		var emptyChallgeSolution pow.ChallengeSolution
		if !allowEmptyChallengeSolution || *req.Challenge != emptyChallgeSolution {

Maybe here we can have an extra if allowEmptyChallengeSolution log: "Warning challenge bypassed do not do this in production"?


functions/internal/report/report.go, line 107 at r2 (raw file):

func StorePendingReport(ctx *util.Context, reportData []byte) (UploadToken, UploadKey, util.StatusError) {
	validityExp := ctx.Now().Add(validityPeriod)
	allocationExp := validityExp.Add(allocationPeriod)

Any benefit to adding a random amount of time to this during run time so they cant be accurately predicted eg:
allocationExp := validityExp.Add(allocationPeriod + rand)

Copy link
Contributor Author

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @joshlf and @madhavajay)


functions/internal/report/report.go, line 107 at r2 (raw file):

Previously, madhavajay (Madhava Jay) wrote…

Any benefit to adding a random amount of time to this during run time so they cant be accurately predicted eg:
allocationExp := validityExp.Add(allocationPeriod + rand)

I'm not aware of any security risk to the allocation period being known. Do you have one in mind?

@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 55b3b56 to 2b8055e Compare May 14, 2020 04:45
Copy link
Contributor Author

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @madhavajay)


functions/report.go, line 77 at r2 (raw file):

Previously, madhavajay (Madhava Jay) wrote…

Maybe here we can have an extra if allowEmptyChallengeSolution log: "Warning challenge bypassed do not do this in production"?

Done.

@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 2b8055e to ec1c6bc Compare May 14, 2020 05:17
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from c0ba7cb to 8f01486 Compare May 14, 2020 05:22
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from ec1c6bc to 46c0425 Compare May 14, 2020 05:22
Copy link
Collaborator

@madhavajay madhavajay left a comment

Choose a reason for hiding this comment

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

:lgtm: This is really solid!

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @madhavajay)

@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from 46c0425 to f1d173b Compare May 14, 2020 05:56
@joshlf joshlf force-pushed the sandbox/joshlf/emulator-unit-test branch from 6e5cb8d to fc14cca Compare May 14, 2020 06:01
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch 2 times, most recently from 01cf583 to b8c87a3 Compare May 14, 2020 06:05
- Split the `util.Context` type into `Context` and `RequestContext`.
  `Context` stores a Firestore client and implements a fake clock
  for testing. `RequestContext` wraps `Context`, and adds HTTP
  request-specific utilities. All code should use a `Context` unless
  it needs HTTP request-specific utilities.
- Implement a fake clock for testing in `util.Context`
- Add utilities to `util.Context` to help run Firestore transactions
@joshlf joshlf force-pushed the sandbox/joshlf/golang-5 branch from b8c87a3 to 53f7ed5 Compare May 14, 2020 06:33
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.

4 participants